[Mlir-commits] [mlir] d8981ce - [mlir][Parser] Fix attribute parser errors for ui64
River Riddle
llvmlistbot at llvm.org
Wed Mar 25 11:57:24 PDT 2020
Author: Frej Drejhammar
Date: 2020-03-25T11:57:16-07:00
New Revision: d8981ce5b9f8caa567613b2bf5aa3095e0156130
URL: https://github.com/llvm/llvm-project/commit/d8981ce5b9f8caa567613b2bf5aa3095e0156130
DIFF: https://github.com/llvm/llvm-project/commit/d8981ce5b9f8caa567613b2bf5aa3095e0156130.diff
LOG: [mlir][Parser] Fix attribute parser errors for ui64
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.
Differential Revision: https://reviews.llvm.org/D76493
Added:
Modified:
mlir/lib/Parser/Parser.cpp
mlir/test/Dialect/SPIRV/canonicalize.mlir
mlir/test/IR/attribute.mlir
mlir/test/IR/invalid.mlir
Removed:
################################################################################
diff --git a/mlir/lib/Parser/Parser.cpp b/mlir/lib/Parser/Parser.cpp
index a29b34b570d0..437803388e99 100644
--- a/mlir/lib/Parser/Parser.cpp
+++ b/mlir/lib/Parser/Parser.cpp
@@ -1759,6 +1759,33 @@ static Optional<APFloat> buildHexadecimalFloatLiteral(Parser *p, FloatType type,
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) {
@@ -1809,19 +1836,12 @@ Attribute Parser::parseDecOrHexAttr(Type type, bool isNegative) {
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
@@ -2038,16 +2058,15 @@ DenseElementsAttr TensorLiteralParser::getIntAttr(llvm::SMLoc loc,
// 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);
diff --git a/mlir/test/Dialect/SPIRV/canonicalize.mlir b/mlir/test/Dialect/SPIRV/canonicalize.mlir
index 72857d7d7de2..f8c3bdebda39 100644
--- a/mlir/test/Dialect/SPIRV/canonicalize.mlir
+++ b/mlir/test/Dialect/SPIRV/canonicalize.mlir
@@ -273,7 +273,7 @@ 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
@@ -331,7 +331,7 @@ 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
diff --git a/mlir/test/IR/attribute.mlir b/mlir/test/IR/attribute.mlir
index 50d4e8cd1454..2be5b623898b 100644
--- a/mlir/test/IR/attribute.mlir
+++ b/mlir/test/IR/attribute.mlir
@@ -33,6 +33,83 @@ func @int_attrs_pass() {
// -----
+//===----------------------------------------------------------------------===//
+// 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"() {
diff --git a/mlir/test/IR/invalid.mlir b/mlir/test/IR/invalid.mlir
index 2c008be930fd..bf4a5fff386a 100644
--- a/mlir/test/IR/invalid.mlir
+++ b/mlir/test/IR/invalid.mlir
@@ -1231,3 +1231,243 @@ 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
+}
More information about the Mlir-commits
mailing list