[MLIR] Fix confusing diagnostic during dialect conversion

[bondhugula]

Summary

Fix confusing diagnostic during partial dialect conversion. A failure to
legalize is not the same as an operation being illegal: for eg. an
operation neither explicity marked legal nor explicitly marked illegal
could have been generated and may have failed to legalize further. The
op isn't an illegal one per
https://mlir.llvm.org/docs/DialectConversion/#conversion-target
which is an op that is explicitly marked illegal.

[mehdi_amini]

The op isn't an illegal one per https://mlir.llvm.org/docs/DialectConversion/#conversion-target which is an op that is explicitly marked illegal.

Can you clarify this? I looked at the doc and your point isn't clear to me.
For example, to me if an op isn't illegal we should not even try to convert it at all.

[mehdi_amini]

I guess something in the doc isn't clear to me: it does not define anything else than legal or illegal as far as I can tell. To me that has been enough to reason about the process, but you're introducing here another state where an operation is neither legal nor illegal?
This third state kind of exists for the purpose of failing the partial legalization I guess, but I can't figure out where it'd be really defined in the section you're linking to?

[bondhugula]

In D116152#3206871, @mehdi_amini wrote:

The op isn't an illegal one per https://mlir.llvm.org/docs/DialectConversion/#conversion-target which is an op that is explicitly marked illegal.

Can you clarify this? I looked at the doc and your point isn't clear to me.
For example, to me if an op isn't illegal we should not even try to convert it at all.

Well, we have a tri-state: explicitly marked legal, explicitly marked illegal, and those that are neither of the former two. The three are treated differently -- for example with a partial conversion, you can end up with success with ops of the last kind but not with those of the second kind.

[mehdi_amini]

Right, I mentioned this in my last message.

Seems like the doc should get updated to reflect this if we want to embrace the "tri-state": right now I see these ops as illegal, they don't make a partial conversion fail.

[mehdi_amini]

I'm fine with changing the debug output though.

[bondhugula]

In D116152#3207077, @mehdi_amini wrote:

Right, I mentioned this in my last message.

Seems like the doc should get updated to reflect this if we want to embrace the "tri-state": right now I see these ops as illegal, they don't make a partial conversion fail.

That's right. The doc needs an update here - it can get quite tricky and confusing the way it's now. I'm happy to tackle that in this revision too. I'll update and will need a review for that.

[bondhugula]

Update dialect conversion doc.

[bondhugula]

Tweak doc update.

[rriddle]

LG, thanks.

mlir/lib/Transforms/Utils/DialectConversion.cpp

1934

nit: failed to legalized <blah>

Feels like ^ would read better, unless what you have matches other messages

[bondhugula]

Improve diagnostic.

[bondhugula]

Adjust other failure messages to be consistent.

mlir/lib/Transforms/Utils/DialectConversion.cpp

1934

Sure, done.

mlir/docs/DialectConversion.md

 
 *   Illegal
 
     -   This action signals that no instance of a given operation is legal.
         Operations marked as "illegal" must always be converted for the
         conversion to be successful. This action also allows for selectively
         marking specific operations as illegal in an otherwise legal dialect.
 
+Operations and dialects that are neither explicitly marked legal nor illegal are
+separate from the above ("unknown" operations) and are treated differently, for
+example, for the purposes of partial conversion as mentioned above.
+
 An example conversion target is shown below:
 
 ```c++
 struct MyTarget : public ConversionTarget {
   MyTarget(MLIRContext &ctx) : ConversionTarget(ctx) {
     //--------------------------------------------------------------------------
     // Marking an operation as Legal:
 

mlir/lib/Transforms/Utils/DialectConversion.cpp

OperationLegalizer::legalizeWithFold(Operation *op,
   rewriter.replaceOp(op, replacementValues);
 
   // Recursively legalize any new constant operations.
   for (unsigned i = curState.numCreatedOps, e = rewriterImpl.createdOps.size();
        i != e; ++i) {
     Operation *cstOp = rewriterImpl.createdOps[i];
     if (failed(legalize(cstOp, rewriter))) {
       LLVM_DEBUG(logFailure(rewriterImpl.logger,
-                            "generated constant '{0}' was illegal",
+                            "failed to legalize generated constant '{0}'",
                             cstOp->getName()));
       rewriterImpl.resetState(curState);
       return failure();
     }
   }
 
   LLVM_DEBUG(logSuccess(rewriterImpl.logger, ""));
   return success();

 
 LogicalResult OperationLegalizer::legalizePatternCreatedOperations(
     ConversionPatternRewriter &rewriter, ConversionPatternRewriterImpl &impl,
     RewriterState &state, RewriterState &newState) {
   for (int i = state.numCreatedOps, e = newState.numCreatedOps; i != e; ++i) {
     Operation *op = impl.createdOps[i];
     if (failed(legalize(op, rewriter))) {
       LLVM_DEBUG(logFailure(impl.logger,
-                            "generated operation '{0}'({1}) was illegal",
+                            "failed to legalize generated operation '{0}'({1})",
                             op->getName(), op));
       return failure();
     }
   }
   return success();
 }
 
 LogicalResult OperationLegalizer::legalizePatternRootUpdates(
     ConversionPatternRewriter &rewriter, ConversionPatternRewriterImpl &impl,
     RewriterState &state, RewriterState &newState) {
   for (int i = state.numRootUpdates, e = newState.numRootUpdates; i != e; ++i) {
     Operation *op = impl.rootUpdates[i].getOperation();
     if (failed(legalize(op, rewriter))) {
-      LLVM_DEBUG(logFailure(impl.logger,
-                            "operation updated in-place '{0}' was illegal",
-                            op->getName()));
+      LLVM_DEBUG(logFailure(
+          impl.logger, "failed to legalize operation updated in-place '{0}'",
       return failure();
     }
   }
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // Cost Model