Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIRRTL] Fix sign bit truncation in constant parser #6794

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

fabianschuiki
Copy link
Contributor

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 fabianschuiki added bug Something isn't working FIRRTL Involving the `firrtl` dialect labels Mar 6, 2024
Copy link
Contributor

@dtzSiFive dtzSiFive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eep! Thanks for fixing this!

test/Dialect/FIRRTL/errors.mlir Outdated Show resolved Hide resolved
if (value.getNumSignBits() < value.getBitWidth() - width)
unsigned neededBits = value.isNegative() ? value.getSignificantBits()
: value.getActiveBits();
if (width < neededBits)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@maerhart
Copy link
Member

maerhart commented Mar 8, 2024

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 fabianschuiki force-pushed the fschuiki/firrtl-fix-const-parser branch from 800dbf4 to cd97a07 Compare March 8, 2024 19:18
@fabianschuiki fabianschuiki merged commit 99968ff into main Mar 8, 2024
4 checks passed
@fabianschuiki fabianschuiki deleted the fschuiki/firrtl-fix-const-parser branch March 8, 2024 20:10
fzi-hielscher added a commit that referenced this pull request Apr 5, 2024
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.
cepheus69 pushed a commit to cepheus69/circt that referenced this pull request Apr 22, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working FIRRTL Involving the `firrtl` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants