Page MenuHomePhabricator

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

Authored by GMNGeoffrey on May 3 2021, 8:42 PM.

Details

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.

Diff Detail

Event Timeline

GMNGeoffrey created this revision.May 3 2021, 8:42 PM
GMNGeoffrey requested review of this revision.May 3 2021, 8:42 PM
GMNGeoffrey edited the summary of this revision. (Show Details)May 3 2021, 8:49 PM
mehdi_amini accepted this revision.May 3 2021, 9:02 PM
mehdi_amini added inline comments.
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

This revision is now accepted and ready to land.May 3 2021, 9:02 PM

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

std.constant is not supposed to support any type.

rriddle requested changes to this revision.May 3 2021, 9:09 PM
This revision now requires changes to proceed.May 3 2021, 9:09 PM

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

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

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)

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

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?

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()

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 edited the summary of this revision. (Show Details)May 4 2021, 12:08 PM
GMNGeoffrey edited the summary of this revision. (Show Details)May 18 2021, 12:54 PM