[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 ↗

(On Diff #413658)

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

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/docs/Diagnostics.md

 op->emitError() << "Compose an interesting error: " << fooAttr << ", " << fooType
                 << ", (" << fooInts << ')';
 
 // This could generate something like (FuncAttr:@foo, IntegerType:i32, {0,1,2}):
 "Compose an interesting error: @foo, i32, (0, 1, 2)"
 ```
 
 Operations attached to a diagnostic will be printed in generic form if the
-severity level is `Error`, otherwise custom operation printers will be used.
+severity level is `Error` or `Warning`, otherwise custom operation printers will
+be used.
 ```c++
 // `anotherOp` will be printed in generic form,
 // e.g. %3 = "arith.addf"(%arg4, %2) : (f32, f32) -> f32
 op->emitError() << anotherOp;
 
 // `anotherOp` will be printed using the custom printer,
 // e.g. %3 = arith.addf %arg4, %2 : f32
 op->emitRemark() << anotherOp;

mlir/docs/OpDefinitions.md

 verified. Verifiers further down the order can rely on certain invariants being
 verified by a previous verifier and do not need to re-verify them.
 
 #### Emitting diagnostics in custom verifiers
 
 Custom verifiers should avoid printing operations using custom operation
 printers, because they require the printed operation (and sometimes its parent
 operation) to be verified first. In particular, when emitting diagnostics,
-custom verifiers should use the `Error` severity level, which prints operations
-in generic form by default, and avoid using lower severity levels (`Note`,
-`Remark`, `Warning`).
+custom verifiers should use the `Error` or `Warning` severity level, which print
+operations in generic form by default, and avoid using lower severity levels
+(`Note`, `Remark`).
 
 ### Declarative Assembly Format
 
 The custom assembly form of the operation may be specified in a declarative
 string that matches the operations operands, attributes, etc. With the ability
 to express additional information that needs to be parsed to build the
 operation:
 

mlir/lib/IR/Diagnostics.cpp

 }
 
 /// Adjusts operation printing flags used in diagnostics for the given severity
 /// level.
 static OpPrintingFlags adjustPrintingFlags(OpPrintingFlags flags,
                                            DiagnosticSeverity severity) {
   flags.useLocalScope();
   flags.elideLargeElementsAttrs();
-  if (severity == DiagnosticSeverity::Error)
+  if (severity == DiagnosticSeverity::Error ||
+      severity == DiagnosticSeverity::Warning)
     flags.printGenericOpForm();
   return flags;
 }
 
 /// Stream in an Operation.
 Diagnostic &Diagnostic::operator<<(Operation &val) {
   return appendOp(val, OpPrintingFlags());
 }