[rtl] Lint fixes for recent Zc code #2357
Conversation
nasahlpa
left a comment
nasahlpa
left a comment
There was a problem hiding this comment.
Thanks Andreas, adding a , should fix the failing CI.
vogelpi
left a comment
vogelpi
left a comment
There was a problem hiding this comment.
Thanks @andreaskurth for the PR, I think we need another fix for the negative offsets as I don't think this is safe and as it doesn't comply with our style guide. See comments for details.
rtl/ibex_compressed_decoder.sv
Outdated
| logic [11:0] neg_offset; | ||
| logic [31:0] instr; | ||
| neg_offset = ~{5'b00000, sp_offset, 2'b00} + 12'd1; | ||
| neg_offset = -{5'b00000, sp_offset, 2'b00}; |
There was a problem hiding this comment.
I am not sure this is okay. I don't think it complies with our style guide, see https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md#signed-arithmetic
The term within { } is probably an unsigned logic and then you invert the sign. How it was before seems safer to me, but I am not sure what the lint warnings/errors were, the width seems fully defined for both operands.
There was a problem hiding this comment.
So according to the style guide maybe this would be a good substitution?
logic signed [11:0] neg_offset;
logic [31:0] instr;
neg_offset = 12'sh000 - signed'({5'b00000, sp_offset, 2'b00});
There was a problem hiding this comment.
Thanks for your feedback. The lint error that gets fixed here is on the explicit calculation of a two's complement, in the form of ~value + 1. I replaced this with -value, as suggested in the linter's manual. I also ran it through simulation to check that the result is correct.
The problem with a signed variable -- that is
logic signed [11:0] neg_offset;
neg_offset = -signed'({5'b00000, sp_offset, 2'b00});is that the linter then complains about the subsequent part selects (e.g., neg_offset[4:0]) of that signed variable.
There was a problem hiding this comment.
Converting back and forth between signed and unsigned should make everyone happy. Not pretty, but let's go with that.
rtl/ibex_compressed_decoder.sv
Outdated
| imm[11:7] = '0; | ||
| imm[ 6:0] = cm_stack_adj(.rlist(rlist), .spimm(spimm)); | ||
| if (decr) imm = ~imm + 12'd1; | ||
| if (decr) imm = -imm; |
There was a problem hiding this comment.
This doesn't look right, you're changing the sign of an unsigned logic, see https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md#signed-arithmetic .
There was a problem hiding this comment.
This is an application of the linter's suggestion, and it behaves correctly in simulation.
If I make imm signed, the linter flags the subsequent assignment to an unsigned target (instr[31:20]) as well as the part selects.
andreaskurth
force-pushed
the
zc-lint-fixes
branch
from
f371d82 to
8ca35f8
Compare
marnovandermaas
left a comment
marnovandermaas
left a comment
There was a problem hiding this comment.
I made a suggestion for dealing with the signed variables.
rtl/ibex_compressed_decoder.sv
Outdated
| logic [11:0] neg_offset; | ||
| logic [31:0] instr; | ||
| neg_offset = ~{5'b00000, sp_offset, 2'b00} + 12'd1; | ||
| neg_offset = -{5'b00000, sp_offset, 2'b00}; |
There was a problem hiding this comment.
So according to the style guide maybe this would be a good substitution?
logic signed [11:0] neg_offset;
logic [31:0] instr;
neg_offset = 12'sh000 - signed'({5'b00000, sp_offset, 2'b00});
Signed-off-by: Andreas Kurth <[email protected]>
andreaskurth
force-pushed
the
zc-lint-fixes
branch
from
8ca35f8 to
516000f
Compare
|
Merging to unblock lowRISC/opentitan#28997. If anyone thinks this should be done differently, we can follow up with another PR. |
4 participants
These were flagged by CI lint checks in lowRISC/opentitan#28997.