[MLIR] Use getValue instead of getInt for std.constant custom asm

[GMNGeoffrey]

Summary

getInt() has a note indicating it is deprecated. The current
behavior with getValue() triggers an assert if the integer is unsigned.
An unsigned integer will hit a verifier failure, but this function is
called on verifier-invalid IR, so needs to be safe against that.

[mehdi_amini]

mlir/lib/Dialect/StandardOps/IR/Ops.cpp

1131

Can we refactor the accessors to access it once?

int64_t value = intCst.getValue().getSExtValue(); around line 1122

[rriddle]

When does unsigned ever happen? You can't have an unsigned constant.

[rriddle]

std.constant is not supposed to support any type.

[rriddle]

Tangentially though, the accessors shouldn't assert based on the type IMO.

[GMNGeoffrey]

In D101810#2735411, @rriddle wrote:

When does unsigned ever happen? You can't have an unsigned constant.

In D101810#2735412, @rriddle wrote:

std.constant is not supposed to support any type.

Then that's a documentation bug :-D https://mlir.llvm.org/docs/Dialects/Standard/#stdconstant-constantop says "any type" on everything. A missing check in the verifier as well. I can send those fixes instead (or as well, since the note about removing the use of getInt() is still there)

In D101810#2735416, @rriddle wrote:

Tangentially though, the accessors shouldn't assert based on the type IMO.

Yeah that also seemed weird to me

mlir/lib/Dialect/StandardOps/IR/Ops.cpp

1131

Only one of these codepaths will be hit, so I don't think it matters much

[GMNGeoffrey]

Oh wait. The verifier *does* check this. This must be as part of printing out the IR for some other reason that I'm hitting this assert?

[GMNGeoffrey]

https://github.com/llvm/llvm-project/blob/959eec1fddc83c90c208789f20cb6573b2a20642/mlir/lib/Dialect/StandardOps/IR/Ops.cpp#L1063

[GMNGeoffrey]

Here's my stack trace (helpfully not symbolized):

@     0x55e5ce041444  __assert_fail
@     0x55e5cde488f1  mlir::IntegerAttr::getInt()
@     0x55e5cdd9b1fd  mlir::ConstantOp::getAsmResultNames()
@     0x55e5cdde46e7  mlir::detail::OpAsmOpInterfaceInterfaceTraits::Model<>::getAsmResultNames()
@     0x55e5cde3c77d  (anonymous namespace)::SSANameState::numberValuesInOp()
@     0x55e5cde3cdd6  (anonymous namespace)::SSANameState::numberValuesInRegion()
@     0x55e5cde406ee  mlir::detail::AsmStateImpl::AsmStateImpl()
@     0x55e5cde3328a  mlir::Operation::print()
@     0x55e5cdebf1c0  mlir::Operation::emitError()
@     0x55e5cdec07c1  mlir::OpState::emitError()
@     0x55e5cddd0a41  mlir::SubTensorOp::verify()
@     0x55e5cddee920  mlir::Op<>::verifyInvariants()
@     0x55e5cdeda2ee  (anonymous namespace)::OperationVerifier::verifyOperation()
@     0x55e5cdeda778  (anonymous namespace)::OperationVerifier::verifyOperation()
@     0x55e5cdeda08f  mlir::verify()
@     0x55e5cdd7d36b  mlir::detail::OpToOpPassAdaptor::run()
@     0x55e5cdd7d7df  mlir::detail::OpToOpPassAdaptor::runPipeline()
@     0x55e5cdd7ea14  mlir::detail::OpToOpPassAdaptor::runOnOperationImpl()
@     0x55e5cdd7d336  mlir::detail::OpToOpPassAdaptor::run()
@     0x55e5cdd7d7df  mlir::detail::OpToOpPassAdaptor::runPipeline()
@     0x55e5cdd7fee8  mlir::PassManager::run()
@     0x55e5cc6f1ec4  mlir::iree_compiler::translateFromMLIRToVMBytecodeModuleWithFlags()

[GMNGeoffrey]

So I think the issue here is that verification is failing, but then it's printing an (unrelated?) constant as part of that and crashing. Probably that constant would fail verification later, but hasn't been verified yet. So I'm a proponent of still making this change, since less crashing is good and getInt() is deprecated. My understanding is that the pretty printer/parsers were supposed to be allowed to assume the op was verified and the verifier was supposed to print generic IR, but I guess that custom asm names are a special case. That would probably be good to investigate and fix more holistically as well

[GMNGeoffrey]

Ping on this?

[GMNGeoffrey]

Just saw this was still outstanding. @rriddle could you take another look? I still think this is an improvement

mlir/lib/Dialect/StandardOps/IR/Ops.cpp

 void ConstantOp::getAsmResultNames(
     function_ref<void(Value, StringRef)> setNameFn) {
   Type type = getType();
   if (auto intCst = getValue().dyn_cast<IntegerAttr>()) {
     IntegerType intTy = type.dyn_cast<IntegerType>();
 
     // Sugar i1 constants with 'true' and 'false'.
     if (intTy && intTy.getWidth() == 1)
-      return setNameFn(getResult(), (intCst.getInt() ? "true" : "false"));
+      return setNameFn(getResult(),
+                       (intCst.getValue().getZExtValue() ? "true" : "false"));
 
     // Otherwise, build a complex name with the value and type.
     SmallString<32> specialNameBuffer;
     llvm::raw_svector_ostream specialName(specialNameBuffer);
-    specialName << 'c' << intCst.getInt();
+    specialName << 'c' << intCst.getValue().getSExtValue();
     if (intTy)
       specialName << '_' << type;
     setNameFn(getResult(), specialName.str());
 
   } else if (type.isa<FunctionType>()) {
     setNameFn(getResult(), "f");
   } else {
     setNameFn(getResult(), "cst");