[mlir] Emit Warning diagnostic in generic form

[Chia-hungDuan]

Summary

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

[Chia-hungDuan]

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?

[rriddle]

In D121174#3365901, @Chia-hungDuan wrote:

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]

The case is here,
https://github.com/tensorflow/tensorflow/blob/fddb6c72caa6a63ec86263a4d4230812c861cd10/tensorflow/compiler/mlir/tensorflow/ir/tf_ops_n_z.cc#L1026

[Chia-hungDuan]

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

[rriddle]

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

mlir/lib/IR/Operation.cpp

257

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

[sgrechanik]

@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]

In D121174#3367765, @sgrechanik wrote:

@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.

[Chia-hungDuan]

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

[rriddle]

mlir/docs/OpDefinitions.md

611 ↗

(On Diff #414221)

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.

mlir/lib/IR/Operation.cpp

InFlightDiagnostic Operation::emitError(const Twine &message) {
   return diag;
 }
 
 /// Emit a warning about this operation, reporting up to any diagnostic
 /// handlers that may be listening.
 InFlightDiagnostic Operation::emitWarning(const Twine &message) {
   InFlightDiagnostic diag = mlir::emitWarning(getLoc(), message);
   if (getContext()->shouldPrintOpOnDiagnostic())
-    diag.attachNote(getLoc()) << "see current operation: " << *this;
+    diag.attachNote(getLoc())
+        .append("see current operation: ")
+        .appendOp(*this, OpPrintingFlags().printGenericOpForm());
   return diag;
 }
 
 /// Emit a remark about this operation, reporting up to any diagnostic
 /// handlers that may be listening.
 InFlightDiagnostic Operation::emitRemark(const Twine &message) {
   InFlightDiagnostic diag = mlir::emitRemark(getLoc(), message);
   if (getContext()->shouldPrintOpOnDiagnostic())
-    diag.attachNote(getLoc()) << "see current operation: " << *this;
+    diag.attachNote(getLoc())
+        .append("see current operation: ")
+        .appendOp(*this, OpPrintingFlags().printGenericOpForm());
   return diag;
 }
 
 //===----------------------------------------------------------------------===//
 // Operation Ordering
 //===----------------------------------------------------------------------===//
 
 constexpr unsigned Operation::kInvalidOrderIdx;