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

[Frej Drejhammar via Phabricator via llvm-commits]

frej added a comment.

Thanks for the review.



================
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();
----------------
rriddle wrote:
> Can you reflow this to be:
> 
> ```
> if (isNegative) {
>   apInt.negate();
> 
>   if (!apInt.isSignbitSet())
>     ...;
> } else if (type.isSignedInteger() || type.isIndex()) && apInt.isSignBitSet()) {
>   ...
> }
> ```
> 
> ?
Will do.


================
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)) {
----------------
rriddle wrote:
> Does the same logic apply here as well?
As far as I can tell, this code appears to have the same problem. I'll move the range checking to its own method and call it from both places.


================
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
 
----------------
rriddle wrote:
> Please fix the now misaligned comments.
Will do


================
Comment at: mlir/test/IR/attribute.mlir:43
+  "test.i8_attr"() {
+    // CHECK: attr = -128 : i8
+    attr = -128 : i8
----------------
rriddle wrote:
> nit: Can you move all of these to one attribute dictionary to reduce the amount of boilerplate here?
Do you mean that you want to see a single operator with multiple attributes?

My first (never sent upstream) version of this patch started out with a single operator looking more or less like the sketch below :

```
def AttrValueValidRangesOp : TEST_Op<"attr_valid_ranges"> {
  let arguments = (ins
    // for each <width> in 8,16,32,64
    AnyI<width>Attr:$i<width>_min, // check that  -(2^(<width>-1)) is accepted
    AnyI<width>Attr:$i<width>_max, // check that 2^(<width>-1)-1 is accepted
    AnyI<width>Attr:$i<width>_wrap_around, // check that 2^<width>-1 is accepted and interpreted as -1 (the correctness of this can be debated, but it is what the unmodified parser does)
    SI<width>Attr:$si<width>_min, // check that  -(2^(<width>-1)) is accepted
    SI<width>Attr:$si<width>_max, // check that 2^(<width>-1)-1 is accepted
    UI<width>Attr:$ui<width>_max // check that 2^(<width>)-1 is accepted
    );
}
```

But then I started thinking about writing the test cases for detecting out of range attributes and discovered that for checking just a single attribute I would have to specify the other 23 attributes. This would greatly inflate the size of these tests as I can only check one attribute at a time. In my rush to get this submitted before the weekend I completely forgot about those kinds of tests. As far as I can tell the current test suite does not systematically test that overflows are detected, so I think that is something that I should add to be sure that I'm not failing to catch errors the existing parser does. I could of course use two operators, one with many attributes for testing valid values, and one with a single attribute for testing invalid values. Would that be palatable?



================
Comment at: mlir/test/lib/TestDialect/TestOps.td:214
 
+def I8AttrValueOp : TEST_Op<"i8_attr"> {
+  let arguments = (ins AnyI8Attr:$attr);
----------------
rriddle wrote:
> None of these are necessary for checking the parser.
Please explain. How can I check that values are valid for an attribute of a given width without having an operator with such an attribute? The best solution I can come up with is a single operator using an APInt attribute:

```
def APIntAttrValueOp : TEST_Op<"apint_attr"> {
  let arguments = (ins APIntAttr:$attr);
}
```

Would this be acceptable or did you have another solution in mind?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76493