[PATCH] D76493: mlir: Fix attribute parser errors for ui64

[River Riddle via Phabricator via llvm-commits]

rriddle requested changes to this revision.
rriddle added a comment.
This revision now requires changes to proceed.

Thanks for the fix!



================
Comment at: mlir/lib/Parser/Parser.cpp:1820
 
-  // Otherwise construct an integer attribute.
-  if (isNegative ? (int64_t)-val.getValue() >= 0 : (int64_t)val.getValue() < 0)
+  if (isNegative)
+    apInt.negate();
----------------
Can you reflow this to be:

```
if (isNegative) {
  apInt.negate();

  if (!apInt.isSignbitSet())
    ...;
} else if (type.isSignedInteger() || type.isIndex()) && apInt.isSignBitSet()) {
  ...
}
```

?


================
Comment at: mlir/lib/Parser/Parser.cpp:2053
     auto val = token.getUInt64IntegerValue();
     if (!val.hasValue() || (isNegative ? (int64_t)-val.getValue() >= 0
                                        : (int64_t)val.getValue() < 0)) {
----------------
Does the same logic apply here as well?


================
Comment at: mlir/test/Dialect/SPIRV/canonicalize.mlir:276
   %c3 = spv.constant 4294967295 : i32  // 2^32 - 1 : 0xffff ffff
-  %c4 = spv.constant -2147483649 : i32 // -2^31 - 1: 0x7fff ffff
+  %c4 = spv.constant 2147483647 : i32 // 2^31 - 1: 0x7fff ffff
 
----------------
Please fix the now misaligned comments.


================
Comment at: mlir/test/IR/attribute.mlir:43
+  "test.i8_attr"() {
+    // CHECK: attr = -128 : i8
+    attr = -128 : i8
----------------
nit: Can you move all of these to one attribute dictionary to reduce the amount of boilerplate here?


================
Comment at: mlir/test/lib/TestDialect/TestOps.td:214
 
+def I8AttrValueOp : TEST_Op<"i8_attr"> {
+  let arguments = (ins AnyI8Attr:$attr);
----------------
None of these are necessary for checking the parser.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76493/new/

https://reviews.llvm.org/D76493