mlir: Fix attribute parser errors for ui64

[frej]

Summary

The attribute parser fails to correctly parse unsigned 64 bit
attributes as the check `isNegative ? (int64_t)-val.getValue() >= 0
: (int64_t)val.getValue() < 0` will falsely detect an overflow for
unsigned values larger than 2^63-1.

This patch reworks the overflow logic to instead of doing arithmetic
on int64_t use APInt::isSignBitSet() and knowledge of the attribute
type.

Test-cases which verify the de-facto behavior of the parser and
triggered the previous faulty handing of unsigned 64 bit attrbutes are
also added.

[rriddle]

Thanks for the fix!

mlir/lib/Parser/Parser.cpp

1841

Can you reflow this to be:

if (isNegative) {
  apInt.negate();

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

?

2060–2061

Does the same logic apply here as well?

mlir/test/Dialect/SPIRV/canonicalize.mlir

276

Please fix the now misaligned comments.

mlir/test/IR/attribute.mlir

43

nit: Can you move all of these to one attribute dictionary to reduce the amount of boilerplate here?

mlir/test/lib/TestDialect/TestOps.td

214 ↗

(On Diff #251622)

None of these are necessary for checking the parser.

[frej]

Thanks for the review.

mlir/lib/Parser/Parser.cpp

1841

Will do.

2060–2061

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.

mlir/test/Dialect/SPIRV/canonicalize.mlir

276

Will do

mlir/test/IR/attribute.mlir

43

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?

mlir/test/lib/TestDialect/TestOps.td

214 ↗

(On Diff #251622)

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?

[rriddle]

mlir/test/lib/TestDialect/TestOps.td

214 ↗

(On Diff #251622)

What I mean is; when you are testing error handling inside of the Parser/AsmPrinter you don't need to register any operations. MLIR doesn't require that you register operations. You could have foo.op and still trigger the necessary code paths inside of the parser/printer.

[frej]

Incorporated River Riddle's review comments and improved the testing of out-of-range values.

[rriddle]

Looks great, I really appreciate the fix!

[frej]

@rriddle : Thanks, are you willing to do the commit as I don't have commit rights? I'd like the change to be attributed to "Frej Drejhammar <[e-mail]>" if possible.

mlir/lib/Parser/Parser.cpp

static Optional<APFloat> buildHexadecimalFloatLiteral(Parser *p, FloatType type,
   APInt apInt(type.getWidth(), value);
   if (apInt != value) {
     p->emitError("hexadecimal float constant out of range for type");
     return llvm::None;
   }
   return APFloat(type.getFloatSemantics(), apInt);
 }
 
+/// Construct an APint from a parsed value, a known attribute type and
+/// sign.
+static Optional<APInt> buildAttributeAPInt(Type type, bool isNegative,
+                                           uint64_t value) {
+  // We have the integer literal as an uint64_t in val, now convert it into an
+  // APInt and check that we don't overflow.
+  int width = type.isIndex() ? 64 : type.getIntOrFloatBitWidth();
+  APInt apInt(width, value, isNegative);
+  if (apInt != value)
+    return llvm::None;
+
+  if (isNegative) {
+    // The value is negative, we have an overflow if the sign bit is not set
+    // in the negated apInt.
+    apInt.negate();
+    if (!apInt.isSignBitSet())
+      return llvm::None;
+  } else if ((type.isSignedInteger() || type.isIndex()) &&
+             apInt.isSignBitSet()) {
+    // The value is a positive signed integer or index,
+    // we have an overflow if the sign bit is set.
+    return llvm::None;
+  }
+
+  return apInt;
+}
+
 /// Parse a decimal or a hexadecimal literal, which can be either an integer
 /// or a float attribute.
 Attribute Parser::parseDecOrHexAttr(Type type, bool isNegative) {
   auto val = getToken().getUInt64IntegerValue();
   if (!val.hasValue())
     return (emitError("integer constant out of range for attribute"), nullptr);
 
   // Remember if the literal is hexadecimal.
return emitError(loc, "integer literal not valid for specified type"),
            nullptr;
 
   if (isNegative && type.isUnsignedInteger()) {
     emitError(loc,
               "negative integer literal not valid for unsigned integer type");
     return nullptr;
   }
 
-  // Parse the integer literal.
-  int width = type.isIndex() ? 64 : type.getIntOrFloatBitWidth();
-  APInt apInt(width, *val, isNegative);
-  if (apInt != *val)
-    return emitError(loc, "integer constant out of range for attribute"),
-           nullptr;
+  Optional<APInt> apInt = buildAttributeAPInt(type, isNegative, *val);
 
-  // Otherwise construct an integer attribute.
-  if (isNegative ? (int64_t)-val.getValue() >= 0 : (int64_t)val.getValue() < 0)
+  if (!apInt)
     return emitError(loc, "integer constant out of range for attribute"),
            nullptr;
-
-  return builder.getIntegerAttr(type, isNegative ? -apInt : apInt);
+  return builder.getIntegerAttr(type, *apInt);
 }
 
 /// Parse elements values stored within a hex etring. On success, the values are
 /// stored into 'result'.
 static ParseResult parseElementAttrHexValues(Parser &parser, Token tok,
                                              std::string &result) {
   std::string val = tok.getStringValue();
   if (val.size() < 2 || val[0] != '0' || val[1] != 'x')
if (token.isAny(Token::kw_true, Token::kw_false)) {
             << "expected i1 type for 'true' or 'false' values";
       APInt apInt(eltTy.getWidth(), token.is(Token::kw_true),
                   /*isSigned=*/false);
       intElements.push_back(apInt);
       continue;
     }
 
     // Create APInt values for each element with the correct bitwidth.
     auto val = token.getUInt64IntegerValue();
-    if (!val.hasValue() || (isNegative ? (int64_t)-val.getValue() >= 0
-                                       : (int64_t)val.getValue() < 0)) {
+    if (!val.hasValue()) {
       p.emitError(tokenLoc, "integer constant out of range for attribute");
       return nullptr;
     }
-    APInt apInt(eltTy.getWidth(), val.getValue(), isNegative);
-    if (apInt != val.getValue())
+    Optional<APInt> apInt = buildAttributeAPInt(eltTy, isNegative, *val);
+    if (!apInt)
       return (p.emitError(tokenLoc, "integer constant out of range for type"),
               nullptr);
-    intElements.push_back(isNegative ? -apInt : apInt);
+    intElements.push_back(*apInt);
   }
 
   return DenseElementsAttr::get(type, intElements);
 }
 
 /// Build a Dense Float attribute for the given type.
 DenseElementsAttr TensorLiteralParser::getFloatAttr(llvm::SMLoc loc,
                                                     ShapedType type,

mlir/test/Dialect/SPIRV/canonicalize.mlir

func @const_fold_scalar_imul_normal() -> (i32, i32, i32) {
   return %0, %1, %2: i32, i32, i32
 }
 
 // CHECK-LABEL: @const_fold_scalar_imul_flow
 func @const_fold_scalar_imul_flow() -> (i32, i32, i32) {
   %c1 = spv.constant 2 : i32
   %c2 = spv.constant 4 : i32
   %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
 
   // (0xffff ffff << 1) = 0x1 ffff fffe -> 0xffff fffe
   // CHECK: %[[CST2:.*]] = spv.constant -2
   %0 = spv.IMul %c1, %c3 : i32
   // (0x7fff ffff << 1) = 0x0 ffff fffe -> 0xffff fffe
   %1 = spv.IMul %c1, %c4 : i32
   // (0x7fff ffff << 2) = 0x1 ffff fffc -> 0xffff fffc
   // CHECK: %[[CST4:.*]] = spv.constant -4
func @const_fold_scalar_isub_normal() -> (i32, i32, i32) {
   return %0, %1, %2: i32, i32, i32
 }
 
 // CHECK-LABEL: @const_fold_scalar_isub_flow
 func @const_fold_scalar_isub_flow() -> (i32, i32, i32, i32) {
   %c1 = spv.constant 0 : i32
   %c2 = spv.constant 1 : i32
   %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     : 0x7fff ffff
   %c5 = spv.constant -1 : i32          //          : 0xffff ffff
   %c6 = spv.constant -2 : i32          //          : 0xffff fffe
 
   // 0x0000 0000 - 0xffff ffff -> 0x0000 0000 + 0x0000 0001 = 0x0000 0001
   // CHECK: spv.constant 1
   %0 = spv.ISub %c1, %c3 : i32
   // 0x0000 0001 - 0xffff ffff -> 0x0000 0001 + 0x0000 0001 = 0x0000 0002
   // CHECK: spv.constant 2

mlir/test/IR/attribute.mlir

"test.int_attrs"() {
     ui32_attr = 6 : ui32
   } : () -> ()
 
   return
 }
 
 // -----
 
+//===----------------------------------------------------------------------===//
+// Check that the maximum and minumum integer attribute values are
+// representable and preserved during a round-trip.
+//===----------------------------------------------------------------------===//
+
+func @int_attrs_pass() {
+  "test.in_range_attrs"() {
+    // CHECK: attr_00 = -128 : i8
+    attr_00 = -128 : i8,
+    // CHECK-SAME: attr_01 = 127 : i8
+    attr_01 = 127 : i8,
+    // CHECK-SAME: attr_02 = -128 : si8
+    attr_02 = -128 : si8,
+    // CHECK-SAME: attr_03 = 127 : si8
+    attr_03 = 127 : si8,
+    // CHECK-SAME: attr_04 = 255 : ui8
+    attr_04 = 255 : ui8,
+    // CHECK-SAME: attr_05 = -32768 : i16
+    attr_05 = -32768 : i16,
+    // CHECK-SAME: attr_06 = 32767 : i16
+    attr_06 = 32767 : i16,
+    // CHECK-SAME: attr_07 = -32768 : si16
+    attr_07 = -32768 : si16,
+    // CHECK-SAME: attr_08 = 32767 : si16
+    attr_08 = 32767 : si16,
+    // CHECK-SAME: attr_09 = 65535 : ui16
+    attr_09 = 65535 : ui16,
+    // CHECK-SAME: attr_10 = -2147483647 : i32
+    attr_10 = -2147483647 : i32,
+    // CHECK-SAME: attr_11 = 2147483646 : i32
+    attr_11 = 2147483646 : i32,
+    // CHECK-SAME: attr_12 = -2147483647 : si32
+    attr_12 = -2147483647 : si32,
+    // CHECK-SAME: attr_13 = 2147483646 : si32
+    attr_13 = 2147483646 : si32,
+    // CHECK-SAME: attr_14 = 4294967295 : ui32
+    attr_14 = 4294967295 : ui32,
+    // CHECK-SAME: attr_15 = -9223372036854775808 : i64
+    attr_15 = -9223372036854775808 : i64,
+    // CHECK-SAME: attr_16 = 9223372036854775807 : i64
+    attr_16 = 9223372036854775807 : i64,
+    // CHECK-SAME: attr_17 = -9223372036854775808 : si64
+    attr_17 = -9223372036854775808 : si64,
+    // CHECK-SAME: attr_18 = 9223372036854775807 : si64
+    attr_18 = 9223372036854775807 : si64,
+    // CHECK-SAME: attr_19 = 18446744073709551615 : ui64
+    attr_19 = 18446744073709551615 : ui64
+  } : () -> ()
+
+  return
+}
+
+// -----
+
+//===----------------------------------------------------------------------===//
+// Check that positive values larger than 2^n-1 for signless integers
+// are mapped to their negative signed counterpart. This behaviour is
+// undocumented in the language specification, but it is what the
+// parser currently does.
+//===----------------------------------------------------------------------===//
+
+func @int_attrs_pass() {
+  "test.i8_attr"() {
+    // CHECK: attr_00 = -1 : i8
+    attr_00 = 255 : i8,
+    // CHECK-SAME: attr_01 = -1 : i16
+    attr_01 = 65535 : i16,
+    // CHECK-SAME: attr_02 = -1 : i32
+    attr_02 = 4294967295 : i32,
+    // CHECK-SAME: attr_03 = -1 : i64
+    attr_03 = 18446744073709551615 : i64
+  } : () -> ()
+  return
+}
+// -----
+
+
 func @wrong_int_attrs_signedness_fail() {
   // expected-error @+1 {{'si32_attr' failed to satisfy constraint: 32-bit signed integer attribute}}
   "test.int_attrs"() {
     any_i32_attr = 5 : i32,
     si32_attr = 7 : ui32,
     ui32_attr = 6 : ui32
   } : () -> ()
   return

mlir/test/IR/invalid.mlir

 }
 
 // -----
 
 func @negative_value_in_unsigned_vector_attr() {
   // expected-error @+1 {{expected unsigned integer elements, but parsed negative value}}
   "foo"() {bar = dense<[5, -5]> : vector<2xui32>} : () -> ()
 }
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = -129 : i8
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = 256 : i8
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = -129 : si8
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = 129 : si8
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{negative integer literal not valid for unsigned integer type}}
+    attr = -1 : ui8
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = 256 : ui8
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = -32769 : i16
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = 65536 : i16
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = -32769 : si16
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = 32768 : si16
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{negative integer literal not valid for unsigned integer type}}
+    attr = -1 : ui16
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = 65536: ui16
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = -2147483649 : i32
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = 4294967296 : i32
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = -2147483649 : si32
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = 2147483648 : si32
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{negative integer literal not valid for unsigned integer type}}
+    attr = -1 : ui32
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = 4294967296 : ui32
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = -9223372036854775809 : i64
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = 18446744073709551616 : i64
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = -9223372036854775809 : si64
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = 9223372036854775808 : si64
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{negative integer literal not valid for unsigned integer type}}
+    attr = -1 : ui64
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @large_bound() {
+  "test.out_of_range_attribute"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr = 18446744073709551616 : ui64
+  } : () -> ()
+  return
+}