Page MenuHomePhabricator

[mlir] Emit Warning diagnostic in generic form
Needs ReviewPublic

Authored by Chia-hungDuan on Mar 7 2022, 4:52 PM.

Details

Summary
In some cases, the custom verifier may use emitWarning to hint a soft
failure. To avoid verifying recursively, use generic form instead.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Mar 7 2022, 4:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 4:52 PM
Chia-hungDuan requested review of this revision.Mar 7 2022, 4:52 PM

Sorry, I'm not following all the discussion in D117834. That CL causes recursively verification in the case that a verifier tries to emitWarning or remark. Any suggestion for the fix?

Sorry, I'm not following all the discussion in D117834. That CL causes recursively verification in the case that a verifier tries to emitWarning or remark. Any suggestion for the fix?

What kind of messages are being emitted as warnings/remarks inside of the verifier?

Chia-hungDuan added a comment.EditedMar 7 2022, 5:05 PM

Also a small snippet of backtrace,
...

#159 0x00005633c4703828 mlir::Operation::print(llvm::raw_ostream&, mlir::OpPrintingFlags const&) 
#160 0x00005633c4756bba mlir::Diagnostic::appendOp(mlir::Operation&, mlir::OpPrintingFlags const&)
#161 0x00005633c4756a9d mlir::Diagnostic::operator<<(mlir::Operation&) 
#162 0x00005633c47af4ac mlir::Operation::emitWarning(llvm::Twine const&)
#163 0x00005633c1a3e6bb mlir::TF::(anonymous namespace)::VerifyShapeOperandAndResult(mlir::Operation*, mlir::Type, mlir::Type, int) tf_ops_n_z.cc:0:0
#164 0x00005633c1a4f650 mlir::TF::VariableShapeOp::verify() 
#165 0x00005633c1742b09 mlir::Op<mlir::TF::VariableShapeOp, mlir::OpTrait::ZeroRegion, mlir::OpTrait::OneResult, mlir::OpTrait::OneTypedResult<mlir::TensorType>::Impl, mlir::OpTrait::ZeroSuccessor, mlir::OpTrait::OneOperand, mlir::OpTrait::OpInvariants, mlir::DerivedAttributeOpInterface::Trait, mlir::MemoryEffectOpInterface::Trait>::verifyInvariants(mlir::Operation*) 
#166 0x00005633b535dff1 mlir::LogicalResult llvm::detail::UniqueFunctionBase<mlir::LogicalResult, mlir::Operation*>::CallImpl<mlir::LogicalResult (* const)(mlir::Operation*)>(void*, mlir::Operation*)
#167 0x00005633c47cd6ae (anonymous namespace)::OperationVerifier::verifyOperation(mlir::Operation&, llvm::SmallVectorImpl<mlir::Operation*>&) Verifier.cpp:0:0
#168 0x00005633c47cdbff (anonymous namespace)::OperationVerifier::verifyOperation(mlir::Operation&, llvm::SmallVectorImpl<mlir::Operation*>&) Verifier.cpp:0:0
#169 0x00005633c47cdbff (anonymous namespace)::OperationVerifier::verifyOperation(mlir::Operation&, llvm::SmallVectorImpl<mlir::Operation*>&) Verifier.cpp:0:0
#170 0x00005633c47cd0c8 (anonymous namespace)::OperationVerifier::verifyOpAndDominance(mlir::Operation&) Verifier.cpp:0:0
#171 0x00005633c47cd069 mlir::verify(mlir::Operation*)
#172 0x00005633c46fcefe mlir::AsmState::AsmState(mlir::Operation*, mlir::OpPrintingFlags const&, llvm::DenseMap<mlir::Operation*, std::__u::pair<unsigned int, unsigned int>, llvm::DenseMapInfo<mlir::Operation*, void>, llvm::detail::DenseMapPair<mlir::Operation*, std::__u::pair<unsigned int, unsigned int> > >*) 
#173 0x00005633c4703828 mlir::Operation::print(llvm::raw_ostream&, mlir::OpPrintingFlags const&) 
#174 0x00005633c4756bba mlir::Diagnostic::appendOp(mlir::Operation&, mlir::OpPrintingFlags const&) 
#175 0x00005633c4756a9d mlir::Diagnostic::operator<<(mlir::Operation&)
#176 0x00005633c47af4ac mlir::Operation::emitWarning(llvm::Twine const&) 
#177 0x00005633c1a3e6bb mlir::TF::(anonymous namespace)::VerifyShapeOperandAndResult(mlir::Operation*, mlir::Type, mlir::Type, int) tf_ops_n_z.cc:0:0
#178 0x00005633c1a4f650 mlir::TF::VariableShapeOp::verify() 
#179 0x00005633c1742b09 mlir::Op<mlir::TF::VariableShapeOp, mlir::OpTrait::ZeroRegion, mlir::OpTrait::OneResult, mlir::OpTrait::OneTypedResult<mlir::TensorType>::Impl, mlir::OpTrait::ZeroSuccessor, mlir::OpTrait::OneOperand, mlir::OpTrait::OpInvariants, mlir::DerivedAttributeOpInterface::Trait, mlir::MemoryEffectOpInterface::Trait>::verifyInvariants(mlir::Operation*) 
#180 0x00005633b535dff1 mlir::LogicalResult llvm::detail::UniqueFunctionBase<mlir::LogicalResult, mlir::Operation*>::CallImpl<mlir::LogicalResult (* const)(mlir::Operation*)>(void*, mlir::Operation*)
#181 0x00005633c47cd6ae (anonymous namespace)::OperationVerifier::verifyOperation(mlir::Operation&, llvm::SmallVectorImpl<mlir::Operation*>&) Verifier.cpp:0:0
#182 0x00005633c47cdbff (anonymous namespace)::OperationVerifier::verifyOperation(mlir::Operation&, llvm::SmallVectorImpl<mlir::Operation*>&) Verifier.cpp:0:0
#183 0x00005633c47cdbff (anonymous namespace)::OperationVerifier::verifyOperation(mlir::Operation&, llvm::SmallVectorImpl<mlir::Operation*>&) Verifier.cpp:0:0
#184 0x00005633c47cd0c8 (anonymous namespace)::OperationVerifier::verifyOpAndDominance(mlir::Operation&) Verifier.cpp:0:0
#185 0x00005633c47cd069 mlir::verify(mlir::Operation*)

If we expand to warnings you'll need to update the docs (as the original commit did).

mlir/lib/IR/Operation.cpp
257 ↗(On Diff #413658)

Do we really need to do that here? I can see expanding to allow warnings, but remarks in verifiers feels weird.

@rriddle Should we also change the adjustPrintingFlags function in mlir/lib/IR/Diagnostics.cpp:130, so that things like emitWarning() << op print op generically as well?

@rriddle Should we also change the adjustPrintingFlags function in mlir/lib/IR/Diagnostics.cpp:130, so that things like emitWarning() << op print op generically as well?

Yeah, that's the place that should really be changed.

I was thinking if we should have a flag to say if we're doing verification to
avoid recursion. After going through the discussion in D117834 now I got it.

In this revision, warning will go though generic form. The remaining will keep
unchanged

Chia-hungDuan retitled this revision from [mlir] Emit Diagnostic in generic form by default to [mlir] Emit Warning diagnostic in generic form.Mar 9 2022, 2:57 PM
Chia-hungDuan edited the summary of this revision. (Show Details)
rriddle added inline comments.Mar 10 2022, 12:34 PM
mlir/docs/OpDefinitions.md
611

Thinking about this in particular, Notes are perfectly fine in verifiers. For operation printing, we'll likely want Note to inherit whatever the parent diagnostic does. We may need to restructure some things to achieve that though. We should likely just emit in the generic form for notes for now and then have an issue/FIXME to inherit the rules of the parent.