Repository navigation
feat(parser): support signed and unsigned integer types - #1414
Conversation
math-fehr
left a comment
math-fehr
left a comment
There was a problem hiding this comment.
I'll review this once you fix the failing CI, but otherwise the design makes sense to me so far!
|
Awesome tysm |
c726ed1 to
a59eee9
Compare
|
Just let me know when you want me to re-review this! |
a59eee9 to
809c90d
Compare
|
Okidokes just rebased - Would really appreicate your review @math-fehr |
math-fehr
left a comment
math-fehr
left a comment
There was a problem hiding this comment.
Nice! Just had 1-2 comments, but otherwise all good!
|
|
||
| #assert expectSuccessType "i32" (IntegerType.mk 32) | ||
| #assert expectSuccessType "i0" (IntegerType.mk 0) | ||
| #assert expectSuccessType "i32" ({ bitwidth := 32 } : IntegerType) |
There was a problem hiding this comment.
Can you change this to IntegerType.signless 32? And similarly in other places?
And can you add IntegerType.signed and IntegerType.unsigned as well as constructors?
There was a problem hiding this comment.
Yes that makes a lot more sense, thank you!
Did I get this right in fad0651
| match resultType with | ||
| | .llvmArrayType arrType => | ||
| if arrType.type ≠ .integerType ⟨8⟩ then | ||
| if arrType.type ≠ .integerType ⟨8, .signless⟩ then |
There was a problem hiding this comment.
Here for instance we could have IntegerType.signless 8 instead of this
| if slice.size < 2 then | ||
| return none | ||
| if (← (getThe ParserState)).input.getD slice.start.byteOffset 0 == 'i'.toUInt8 then | ||
| let input := (← (getThe ParserState)).input |
There was a problem hiding this comment.
Here, can you instead use slice.of ((← getThe ParserState).input) to get a ByteArray, and then just check equality with "si".toByteArray and so-on. That should make the code simpler I think.
There was a problem hiding this comment.
Ah this made it a lot simpler. Thank you! Yes way simpler.
|
Oh also, a lot of the verifiers are not incorrect, since they now allow unsigned and signed integers, while they should not. |
809c90d to
9bfbf25
Compare
|
Done in c9f5099 with some new regression tests |
e6d9670 to
a1a20e7
Compare
|
Arg sorry, all good now, but I forgot about this and now there is a merge conflict. Could you rebase it on main and fix the merge conflicts? Otherwise I can do this on your branch if you want, sorry about that! |
a1a20e7 to
60c287c
Compare
|
No worries and thank you very much! @math-fehr |
|
Thanks for reviewing @tobiasgrosser @math-fehr |
|
Is this ready to merge? |
Replace `{ bitwidth := n }` with `IntegerType.signless n` throughout,
and add `IntegerType.signed` and `IntegerType.unsigned` constructors.
…ntegerType Only signless integers are valid operand and result types for the operations that apply this check, matching MLIR's signless-fixed-width-integer-like constraint.
- Use `IntegerType.signless` in unit tests added upstream. - Print `true`/`false` only for signless `i1`, as MLIR does. - Normalize integer attribute values of unsigned types into the unsigned range, so `255 : ui8` round-trips. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
60c287c to
7102aa8
Compare
|
I rebased this and merge this now. |
3 participants
Fixes #1352.
MLIR integer types carry signedness:
i32(signless),si32(signed),ui32(unsigned). Veir only supportedi32.Adds a
Signednessfield toIntegerTypedefaulting to.signless, so all existing code compiles without change beyond updating constructor call sites.