This is an archive of the discontinued LLVM Phabricator instance.

Differential D129930

[mlir][Arithmetic] Fix printing larger integer attributes in arith.const
ClosedPublic

Authored by mhillenbrand on Jul 16 2022, 4:05 AM.

Details

Reviewers
rriddle
mehdi_amini
ftynse
bondhugula
Commits
rG1ef32e78284b: [mlir][Arithmetic] Fix printing larger integer attributes in arith.const
Summary

For arith.constant operations of integer type, the operation generates
result names that include the value of the constant (i.e., the
IntegerAttr that defines the constant's value). That code currently
assumes integer widths of 64 bits or less and hits an assert with wider
constants or would create truncated and potentially ambiguous names when
built with assertions disabled.

To enable printing arith.constant ops for arbitrarily wide integer
types, change to use the IntegerAttr's function getValue() when
generating result names.

Also, add a regression test.

Diff Detail

Event Timeline

mhillenbrand created this revision.Jul 16 2022, 4:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2022, 4:05 AM
Herald added subscribers: bzcheeseman, sdasgup3, wenzhicui and 18 others. · View Herald Transcript
mhillenbrand requested review of this revision.Jul 16 2022, 4:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2022, 4:05 AM
Herald added subscribers: stephenneuendorffer, nicolasvasilache. · View Herald Transcript
mhillenbrand added a comment.Jul 16 2022, 4:31 AM

For reference, without the fix, the constants added in test/Dialect/Arithmetic/ops.mlir will fail an assertion as in:

mlir-opt: .../git/llvm-project/llvm/include/llvm/ADT/APInt.h:1481: int64_t llvm::APInt::getSExtValue() const: Assertion `getSignificantBits() <= 64 && "Too many bits for int64_t"' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.      Program arguments: ../../../../build/bin/mlir-opt ops.mlir
 #0 0x0000000002b4d6db llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) .../git/llvm-project/llvm/lib/Support/Unix/Signals.inc:569:22
 #1 0x0000000002b4d79e PrintStackTraceSignalHandler(void*) .../git/llvm-project/llvm/lib/Support/Unix/Signals.inc:636:1
 #2 0x0000000002b4b604 llvm::sys::RunSignalHandlers() .../git/llvm-project/llvm/lib/Support/Signals.cpp:103:20
 #3 0x0000000002b4d0f9 SignalHandler(int) .../git/llvm-project/llvm/lib/Support/Unix/Signals.inc:407:1
 #4 0x00007fb4a63daa70 __restore_rt (/lib64/libc.so.6+0x3ea70)
 #5 0x00007fb4a642ac4c __pthread_kill_implementation (/lib64/libc.so.6+0x8ec4c)
 #6 0x00007fb4a63da9c6 gsignal (/lib64/libc.so.6+0x3e9c6)
 #7 0x00007fb4a63c47f4 abort (/lib64/libc.so.6+0x287f4)
 #8 0x00007fb4a63c471b _nl_load_domain.cold (/lib64/libc.so.6+0x2871b)
 #9 0x00007fb4a63d3576 (/lib64/libc.so.6+0x37576)
#10 0x0000000002a8dc33 llvm::APInt::getSExtValue() const .../git/llvm-project/llvm/include/llvm/ADT/APInt.h:1482:22
#11 0x0000000005509358 mlir::IntegerAttr::getInt() const .../git/llvm-project/mlir/lib/IR/BuiltinAttributes.cpp:336:33
#12 0x0000000002cefb56 mlir::arith::ConstantOp::getAsmResultNames(llvm::function_ref<void (mlir::Value, llvm::StringRef)>) .../git/llvm-project/mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp:100:41
#13 0x0000000002dc6ad7 mlir::detail::OpAsmOpInterfaceInterfaceTraits::Model<mlir::arith::ConstantOp>::getAsmResultNames(mlir::detail::OpAsmOpInterfaceInterfaceTraits::Concept const*, mlir::Operation*, llvm::function_ref<void (mlir::Value, llvm::StringRef)>) .../git/llvm-project/build/tools/mlir/include/mlir/IR/OpAsmInterface.h.inc:83:83
#14 0x00000000054c64f9 mlir::OpAsmOpInterface::getAsmResultNames(llvm::function_ref<void (mlir::Value, llvm::StringRef)>) .../git/llvm-project/build/tools/mlir/include/mlir/IR/OpAsmInterface.cpp.inc:10:79
#15 0x00000000054c9eed (anonymous namespace)::SSANameState::numberValuesInOp(mlir::Operation&) .../git/llvm-project/mlir/lib/IR/AsmPrinter.cpp:1165:41
#16 0x00000000054c9a31 (anonymous namespace)::SSANameState::numberValuesInBlock(mlir::Block&) .../git/llvm-project/mlir/lib/IR/AsmPrinter.cpp:1126:19
#17 0x00000000054c981a (anonymous namespace)::SSANameState::numberValuesInRegion(mlir::Region&) .../git/llvm-project/mlir/lib/IR/AsmPrinter.cpp:1092:22
#18 0x00000000054c8ba6 (anonymous namespace)::SSANameState::SSANameState(mlir::Operation*, mlir::OpPrintingFlags const&) .../git/llvm-project/mlir/lib/IR/AsmPrinter.cpp:980:40
...
Harbormaster completed remote builds in B175817: Diff 445224.Jul 16 2022, 4:34 AM
mhillenbrand added reviewers: rriddle, mehdi_amini, ftynse.Jul 16 2022, 4:46 AM
jpienaar added a subscriber: jpienaar.Jul 16 2022, 7:24 AM
jpienaar added inline comments.
mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp
94

With your change this special case becomes redundant and could be removed here.

bondhugula accepted this revision.Jul 16 2022, 9:47 AM
bondhugula added a subscriber: bondhugula.

Nice catch.

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp
94

How does this become redundant (assuming you still want i1 to appear as true/false)? The snippet below would prefix a c.

101

Unrelated: it isn't clear why an arith.constant op with an IntegerAttr would have a result type other than an int type. Perhaps, this is a remnant of its previous presence in the std dialect. In a separate revision, this guard on intType being non-null here and at line 94 can be removed.

This revision is now accepted and ready to land.Jul 16 2022, 9:47 AM
jpienaar added inline comments.Jul 17 2022, 3:00 PM
mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp
94

You are correct, the prefix would be additional and unrelated to fixing crash

mhillenbrand added a comment.Jul 18 2022, 12:53 AM

@bondhugula Thanks for your review and approval. Could you please commit this change?

bondhugula added a comment.Jul 24 2022, 8:08 PM
In D129930#3658929, @mhillenbrand wrote:

@bondhugula Thanks for your review and approval. Could you please commit this change?

Sure. Sorry about the delay here.

Closed by commit rG1ef32e78284b: [mlir][Arithmetic] Fix printing larger integer attributes in arith.const (authored by mhillenbrand, committed by bondhugula). · Explain WhyJul 24 2022, 8:16 PM
This revision was automatically updated to reflect the committed changes.
bondhugula added a commit: rG1ef32e78284b: [mlir][Arithmetic] Fix printing larger integer attributes in arith.const.
mhillenbrand added a comment.Jul 26 2022, 5:25 AM

Thanks for merging!

Revision Contents

PathSize
mlir/
lib/
Dialect/
Arithmetic/
IR/
4 lines
test/
Dialect/
Arithmetic/
6 lines

Diff 445224

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp

Show First 20 Lines • Show All 85 Lines • ▼ Show 20 Lines
void arith::ConstantOp::getAsmResultNames( void arith::ConstantOp::getAsmResultNames(
function_ref<void(Value, StringRef)> setNameFn) { function_ref<void(Value, StringRef)> setNameFn) {
auto type = getType(); auto type = getType();
if (auto intCst = getValue().dyn_cast<IntegerAttr>()) { if (auto intCst = getValue().dyn_cast<IntegerAttr>()) {
auto intType = type.dyn_cast<IntegerType>(); auto intType = type.dyn_cast<IntegerType>();
// Sugar i1 constants with 'true' and 'false'. // Sugar i1 constants with 'true' and 'false'.
if (intType && intType.getWidth() == 1) if (intType && intType.getWidth() == 1)
jpienaarUnsubmitted
Not Done
ReplyInline Actions

With your change this special case becomes redundant and could be removed here.

jpienaar: With your change this special case becomes redundant and could be removed here.
bondhugulaUnsubmitted
Not Done
ReplyInline Actions

How does this become redundant (assuming you still want i1 to appear as true/false)? The snippet below would prefix a c.

bondhugula: How does this become redundant (assuming you still want `i1` to appear as true/false)? The…
jpienaarUnsubmitted
Not Done
ReplyInline Actions

You are correct, the prefix would be additional and unrelated to fixing crash

jpienaar: You are correct, the prefix would be additional and unrelated to fixing crash
return setNameFn(getResult(), (intCst.getInt() ? "true" : "false")); return setNameFn(getResult(), (intCst.getInt() ? "true" : "false"));
// Otherwise, build a compex name with the value and type. // Otherwise, build a complex name with the value and type.
SmallString<32> specialNameBuffer; SmallString<32> specialNameBuffer;
llvm::raw_svector_ostream specialName(specialNameBuffer); llvm::raw_svector_ostream specialName(specialNameBuffer);
specialName << 'c' << intCst.getInt(); specialName << 'c' << intCst.getValue();
if (intType) if (intType)
bondhugulaUnsubmitted
Not Done
ReplyInline Actions

Unrelated: it isn't clear why an arith.constant op with an IntegerAttr would have a result type other than an int type. Perhaps, this is a remnant of its previous presence in the std dialect. In a separate revision, this guard on intType being non-null here and at line 94 can be removed.

bondhugula: Unrelated: it isn't clear why an `arith.constant` op with an `IntegerAttr` would have a result…
specialName << '_' << type; specialName << '_' << type;
setNameFn(getResult(), specialName.str()); setNameFn(getResult(), specialName.str());
} else { } else {
setNameFn(getResult(), "cst"); setNameFn(getResult(), "cst");
} }
} }
/// TODO: disallow arith.constant to return anything other than signless integer /// TODO: disallow arith.constant to return anything other than signless integer
▲ Show 20 Lines • Show All 1,918 Lines • Show Last 20 Lines

mlir/test/Dialect/Arithmetic/ops.mlir

Show First 20 Lines • Show All 918 Lines • ▼ Show 20 Linesfunc.func @test_constant() -> () {
%6 = arith.constant dense<0> : vector<42 x i32> %6 = arith.constant dense<0> : vector<42 x i32>
// CHECK: %true = arith.constant true // CHECK: %true = arith.constant true
%7 = arith.constant true %7 = arith.constant true
// CHECK: %false = arith.constant false // CHECK: %false = arith.constant false
%8 = arith.constant false %8 = arith.constant false
// CHECK: %c-1_i128 = arith.constant -1 : i128
%9 = arith.constant 340282366920938463463374607431768211455 : i128
// CHECK: %c85070591730234615865843651857942052864_i128 = arith.constant 85070591730234615865843651857942052864 : i128
%10 = arith.constant 85070591730234615865843651857942052864 : i128
return return
} }
// CHECK-LABEL: func @maximum // CHECK-LABEL: func @maximum
func.func @maximum(%v1: vector<4xf32>, %v2: vector<4xf32>, func.func @maximum(%v1: vector<4xf32>, %v2: vector<4xf32>,
%sv1: vector<[4]xf32>, %sv2: vector<[4]xf32>, %sv1: vector<[4]xf32>, %sv2: vector<[4]xf32>,
%f1: f32, %f2: f32, %f1: f32, %f2: f32,
%i1: i32, %i2: i32) { %i1: i32, %i2: i32) {
Show All 20 Lines