[Mlir-commits] [mlir] 5cd0b81 - [mlir] Allow IntegerAttr to parse zero width integers.

[Stella Laurenzo]

Author: Stella Laurenzo
Date: 2021-12-30T20:33:00-08:00
New Revision: 5cd0b817e2398d9643f74728970fe4c65776c012

URL: https://github.com/llvm/llvm-project/commit/5cd0b817e2398d9643f74728970fe4c65776c012
DIFF: https://github.com/llvm/llvm-project/commit/5cd0b817e2398d9643f74728970fe4c65776c012.diff

LOG: [mlir] Allow IntegerAttr to parse zero width integers.

https://reviews.llvm.org/D109555 added support to APInt for this, so the special case to disable it is no longer valid. It is in fact legal to construct these programmatically today, and they print properly but do not parse.

Justification: zero bit integers arise naturally in various bit reduction optimization problems, and having them defined for MLIR reduces special casing.

I think there is a solid case for i0 and ui0 being supported. I'm less convinced about si0 and opted to just allow the parser to round-trip values that already verify. The counter argument is that the proper singular value for an si0 is -1. But the counter to this counter is that the sign bit is N-1, which does not exist for si0 and it is not unreasonable to consider this non-existent bit to be 0. Various sources consider it having the singular value "0" to be the least surprising.

Reviewed By: lattner

Differential Revision: https://reviews.llvm.org/D116413

Added: 
    

Modified: 
    mlir/lib/Parser/AttributeParser.cpp
    mlir/test/IR/attribute.mlir
    mlir/test/IR/invalid-ops.mlir

Removed: 
    


################################################################################
diff  --git a/mlir/lib/Parser/AttributeParser.cpp b/mlir/lib/Parser/AttributeParser.cpp
index 881e5b6d0e6db..d7087de8aad90 100644
--- a/mlir/lib/Parser/AttributeParser.cpp
+++ b/mlir/lib/Parser/AttributeParser.cpp
@@ -335,10 +335,6 @@ static Optional<APInt> buildAttributeAPInt(Type type, bool isNegative,
   unsigned width = type.isIndex() ? IndexType::kInternalStorageBitWidth
                                   : type.getIntOrFloatBitWidth();
 
-  // APInt cannot hold a zero bit value.
-  if (width == 0)
-    return llvm::None;
-
   if (width > result.getBitWidth()) {
     result = result.zext(width);
   } else if (width < result.getBitWidth()) {
@@ -350,7 +346,12 @@ static Optional<APInt> buildAttributeAPInt(Type type, bool isNegative,
     result = result.trunc(width);
   }
 
-  if (isNegative) {
+  if (width == 0) {
+    // 0 bit integers cannot be negative and manipulation of their sign bit will
+    // assert, so short-cut validation here.
+    if (isNegative)
+      return llvm::None;
+  } else if (isNegative) {
     // The value is negative, we have an overflow if the sign bit is not set
     // in the negated apInt.
     result.negate();

diff  --git a/mlir/test/IR/attribute.mlir b/mlir/test/IR/attribute.mlir
index f8c07e37b63dc..63c05c0cd057b 100644
--- a/mlir/test/IR/attribute.mlir
+++ b/mlir/test/IR/attribute.mlir
@@ -150,8 +150,54 @@ func @int_attrs_pass() {
   } : () -> ()
   return
 }
+
 // -----
 
+//===----------------------------------------------------------------------===//
+// Check that i0 is parsed and verified correctly. It can only have value 0.
+// We check it explicitly because there are various special cases for it that
+// are good to verify.
+//===----------------------------------------------------------------------===//
+
+func @int0_attrs_pass() {
+  "test.i0_attr"() {
+    // CHECK: attr_00 = 0 : i0
+    attr_00 = 0 : i0,
+    // CHECK: attr_01 = 0 : si0
+    attr_01 = 0 : si0,
+    // CHECK: attr_02 = 0 : ui0
+    attr_02 = 0 : ui0,
+    // CHECK: attr_03 = 0 : i0
+    attr_03 = 0x0000 : i0,
+    // CHECK: attr_04 = 0 : si0
+    attr_04 = 0x0000 : si0,
+    // CHECK: attr_05 = 0 : ui0
+    attr_05 = 0x0000 : ui0
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @int0_attrs_negative_fail() {
+  "test.i0_attr"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr_00 = -1 : i0
+  } : () -> ()
+  return
+}
+
+// -----
+
+func @int0_attrs_positive_fail() {
+  "test.i0_attr"() {
+    // expected-error @+1 {{integer constant out of range for attribute}}
+    attr_00 = 1 : i0
+  } : () -> ()
+  return
+}
+
+// -----
 
 func @wrong_int_attrs_signedness_fail() {
   // expected-error @+1 {{'si32_attr' failed to satisfy constraint: 32-bit signed integer attribute}}

diff  --git a/mlir/test/IR/invalid-ops.mlir b/mlir/test/IR/invalid-ops.mlir
index 2aae390af4d02..09237ccf036de 100644
--- a/mlir/test/IR/invalid-ops.mlir
+++ b/mlir/test/IR/invalid-ops.mlir
@@ -187,11 +187,3 @@ func @atomic_yield_type_mismatch(%I: memref<10xf32>, %i : index) {
   }
   return
 }
-
-// -----
-
-func @no_zero_bit_integer_attrs() {
-  // expected-error @+1 {{integer constant out of range for attribute}}
-  %x = "some.op"(){value = 0 : i0} : () -> f32
-  return
-}