[FIRRTL] Fix sign bit truncation in constant parser #6794
Conversation
fabianschuiki
added
bug
fabianschuiki
requested review from
darthscsi,
dtzSiFive and
seldridge
as code owners
| if (value.getNumSignBits() < value.getBitWidth() - width) | ||
| unsigned neededBits = value.isNegative() ? value.getSignificantBits() | ||
| : value.getActiveBits(); | ||
| if (width < neededBits) |
There was a problem hiding this comment.
Are we actually allowed to remove the leading zero if it is a positive value (which would happen here because getActiveBits == bitWidth - numLeadingZeros)? I think the APInt is just used as a signless bitvector and the return type actually determines how this bitvector should be interpreted. If the type is sint then it's two's complement. E.g., we have the binary number 01 (1 in decimal) and we truncate it to 1 then it's -1 in decimal. 1 (decimal) cannot be represented in a 1-bit signed integer. If the APInt is interpreted as unsigned, then we can truncate to 1.
There was a problem hiding this comment.
I think it makes sense to look at the parsed value and see if that value as it was parsed can be truncated to the bit width. If it's positive it has a leading 0 bit (the binary 0b10 actually parses into APInt 010), and if it's negative it has a leading 1 bit. That means we can always check if the user typed a negative or positive value. Then this check ensures that by cutting off leading bits we don't change the interpretation of the number, or rather, that there is no overflow.
|
Btw. please don't wait for me, just dumped my thoughts 😅 |
Fix an issue in the parser of the `firrtl.constant` op, which would truncate the sign bit of negative constants without reporting an error. This would for example accept -2 as a valid 1-bit constant, truncating the `...11111110` bit pattern to `0`.
fabianschuiki
force-pushed
the
fschuiki/firrtl-fix-const-parser
branch
from
800dbf4
to
cd97a07
Compare
Some minor changes to the construction of BitVector attributes in the SMT dialect: - Fix parsing of smt.bv.constant #smt.bv<-1> : !smt.bv<1> which currently trips the width check due to odsParser.parseInteger creating an unnecessarily wide APInt. The new logic is copy-pasted from the FIRRTL ConstantOp parser. See #6794. - Change the type of the attribute builder's value argument from unsigned to uint64_t matching the signature of the APInt constructor, to allow values up to 64 bits and avoid architecture dependent behavior. - Prevent left-shifts wider than (or equal to) the shifted operand's number of bits in the width checking logic to avoid undefined behavior.
Some minor changes to the construction of BitVector attributes in the SMT dialect: - Fix parsing of smt.bv.constant #smt.bv<-1> : !smt.bv<1> which currently trips the width check due to odsParser.parseInteger creating an unnecessarily wide APInt. The new logic is copy-pasted from the FIRRTL ConstantOp parser. See llvm#6794. - Change the type of the attribute builder's value argument from unsigned to uint64_t matching the signature of the APInt constructor, to allow values up to 64 bits and avoid architecture dependent behavior. - Prevent left-shifts wider than (or equal to) the shifted operand's number of bits in the width checking logic to avoid undefined behavior.
Fix an issue in the parser of the
firrtl.constantop, which would truncate the sign bit of negative constants without reporting an error. This would for example accept -2 as a valid 1-bit constant, truncating the...11111110bit pattern to0.