[mlir] Fix bug in partial dialect conversion

[vinograd47]

Summary

The discussion on forum:
https://llvm.discourse.group/t/bug-in-partial-dialect-conversion/4115

The applyPartialConversion didn't handle the operations, that were
marked as illegal inside dynamic legality callback.
Instead of reporting error, if such operation was not converted to legal set,
the method just added it to unconvertedSet in the same way as unknown operations.

This patch fixes that and handle dynamically illegal operations as well.

The patch includes 2 fixes for existing passes:

• tensor-bufferize - explicitly mark std.return as legal.
• convert-parallel-loops-to-gpu - ugly fix with marking visited operations to avoid recursive legality checks.

[bondhugula]

mlir/include/mlir/Dialect/GPU/ParallelLoopMapper.h

45

Do you need to expose this? Just make it local to SCFToGPU.cpp.

mlir/lib/Conversion/SCFToGPU/SCFToGPU.cpp

571

rewriter.getUnitAttr()

[vinograd47]

Applied review remarks.

[vinograd47]

@bondhugula I've fixed your remarks.

[rriddle]

Thanks for the patch!

mlir/lib/Conversion/SCFToGPU/SCFToGPU.cpp

570

Can you describe the logic behind this? It feels a bit sketchy.

mlir/lib/Transforms/Utils/DialectConversion.cpp

1547

I don't think this function name needs to change.

mlir/test/Transforms/test-legalizer.mlir

302–311

Please format the other tests like this one.

mlir/test/lib/Dialect/Test/TestOps.td

1326

UnregisteredOp is a confusing name for an operation that is registered. I'd suggest using a different/existing operation, or name this something else.

[vinograd47]

Fixed review remarks

[vinograd47]

mlir/lib/Conversion/SCFToGPU/SCFToGPU.cpp

570

Added extra comments to the kVisitedAttrName constant declaration with logic explanation.

mlir/test/lib/Dialect/Test/TestOps.td

1326

Replaced it with std.constant.

[vinograd47]

Fix review remarks

[vinograd47]

@rriddle I've fixed your remarks

[rriddle]

Approval for the general bits, but I'd like sign off from someone in the GPU space (e.g. @ftynse or @herhut ) as they would be the ones following up on (and possibly addressing in some other form in the future) the SCFToGPU.cpp parallel loop marker fix.

[vinograd47]

@ftynse @herhut Please take a look into GPU related part.

[ftynse]

Sorry for the delay: once approved by someone, the diff doesn't show in the review chain anymore, don't hesitate to ping me on other channels if necessary.

Are there cases (e.g., tests) where we expect the parallel loops with unsupported mapping to persist in the IR or are we always erroring out? In the latter case, just keeping the unlowered op as illegal would still error out but with a different message (conversion failed), we can just update the message in tests in that case.

mlir/lib/Conversion/SCFToGPU/SCFToGPU.cpp

42

There's a second criterion, right?

50

Nit: consider adding a note, maybe a TODO, that a cleaner solution would require factoring out the "matching" logic from the pattern and its callees into a separate function that can be called from both the pattern and the op legality check.

[vinograd47]

Rebased and extend NOTE/TODO comment

[vinograd47]

In D108505#2986623, @ftynse wrote:

Are there cases (e.g., tests) where we expect the parallel loops with unsupported mapping to persist in the IR or are we always erroring out? In the latter case, just keeping the unlowered op as illegal would still error out but with a different message (conversion failed), we can just update the message in tests in that case.

Yes, there are such cases and they started to fail after my original fixes to dialect conversion engine. That's the reason why I've added such workaround to the SCFToGPU logic.

I've updated the comment in the pass with TODO note.

mlir/lib/Conversion/SCFToGPU/SCFToGPU.cpp

42

There's a second criterion, right?

42

There was only one criteria in the legality callback. The more advanced check is done in rewriter.

[akuegel]

It looks like there are some integration tests which have not been adapted to this change, e.g.

mlir/test/Integration/Dialect/Linalg/CPU/test-collapse-tensor.mlir

When running cmake, you can specify -DMLIR_INCLUDE_INTEGRATION_TESTS=ON to get them included, and then run the check-mlir target.
I think this change should be temporarily reverted to make the tests succeed again.

[ftynse]

There are more tests that are failing:

Failed Tests (8):
  MLIR :: Integration/Dialect/Linalg/CPU/test-collapse-tensor.mlir
  MLIR :: Integration/Dialect/Linalg/CPU/test-elementwise.mlir
  MLIR :: Integration/Dialect/Linalg/CPU/test-expand-tensor.mlir
  MLIR :: Integration/Dialect/Linalg/CPU/test-padtensor.mlir
  MLIR :: Integration/Dialect/Linalg/CPU/test-subtensor-insert-multiple-uses.mlir
  MLIR :: Integration/Dialect/Linalg/CPU/test-subtensor-insert.mlir
  MLIR :: Integration/Dialect/Linalg/CPU/test-tensor-e2e.mlir
  MLIR :: Integration/Dialect/Linalg/CPU/test-tensor-matmul.mlir

[vinograd47]

@akuegel Yes, I'm currently investigating them.

[vinograd47]

@akuegel @ftynse I've just pushed a fix to upstream.

mlir/include/mlir/Conversion/SCFToGPU/SCFToGPU.h

 #include "mlir/Support/LLVM.h"
 
 namespace mlir {
 class AffineForOp;
 class ConversionTarget;
 struct LogicalResult;
 class MLIRContext;
 class Value;
+class Operation;
 class RewritePatternSet;
 using OwningRewritePatternList = RewritePatternSet;
 
 namespace scf {
 class ForOp;
 } // end namespace scf
 
 /// Convert a perfect affine loop nest with the outermost loop identified by

 /// Adds the conversion pattern from `scf.parallel` to `gpu.launch` to the
 /// provided pattern list.
 void populateParallelLoopToGPUPatterns(RewritePatternSet &patterns);
 
 /// Configures the rewrite target such that only `scf.parallel` operations that
 /// are not rewritten by the provided patterns are legal.
 void configureParallelLoopToGPULegality(ConversionTarget &target);
 
+/// Clean up after applyPartialConversion/applyFullConversion call.
+void finalizeParallelLoopToGPUConversion(Operation *op);
+
 } // namespace mlir
 
 #endif // MLIR_CONVERSION_SCFTOGPU_SCFTOGPU_H_

mlir/include/mlir/Dialect/GPU/ParallelLoopMapper.h

 class ParallelOp;
 }
 
 namespace gpu {
 
 /// Name of the mapping attribute produced by loop mappers.
 StringRef getMappingAttrName();
 
+/// Name of internal attribute to mark visited operations during conversion.
+StringRef getVisitedAttrName();
+
 /// Get the value of the processor in the ParallelLoopDimMapping attribute.
 inline Processor getProcessor(ParallelLoopDimMapping attr) {
   return static_cast<Processor>(attr.processor().getInt());
 }
 
 /// Helper function to create a ParallelDimMapperAttr.
 /// TODO: Replace its uses with an auto-gened method.
 ParallelLoopDimMapping getParallelLoopDimMappingAttr(Processor processor,

mlir/lib/Conversion/SCFToGPU/SCFToGPU.cpp

 
 #define DEBUG_TYPE "loops-to-gpu"
 
 using namespace mlir;
 using namespace mlir::scf;
 
 // Extract an indexed value from KernelDim3.
 static Value getDim3Value(const gpu::KernelDim3 &dim3, unsigned pos) {
   switch (pos) {
   case 0:
     return dim3.x;
   case 1:
     return dim3.y;
   case 2:
     return dim3.z;
   default:
     llvm_unreachable("dim3 position out of bounds");
   }
   return nullptr;
 }
 
 // Get the lower bound-related operands of a loop operation.
 static Operation::operand_range getLowerBoundOperands(AffineForOp forOp) {
   return forOp.getLowerBoundOperands();
 }

 /// the hardware id might iterate over additional indices. The transformation
 /// caters for this by predicating the created sequence of instructions on
 /// the actual loop bound. This only works if an static upper bound for the
 /// dynamic loop bound can be derived, currently via analyzing `affine.min`
 /// operations.
 LogicalResult
 ParallelToGpuLaunchLowering::matchAndRewrite(ParallelOp parallelOp,
                                              PatternRewriter &rewriter) const {
+  // Mark the operation as visited for recursive legality check.
+  parallelOp->setAttr(gpu::getVisitedAttrName(), UnitAttr::get(getContext()));
+
   // We can only transform starting at the outer-most loop. Launches inside of
   // parallel loops are not supported.
   if (auto parentLoop = parallelOp->getParentOfType<ParallelOp>())
     return failure();
   // Create a launch operation. We start with bound one for all grid/block
   // sizes. Those will be refined later as we discover them from mappings.
   Location loc = parallelOp.getLoc();
   Value constantOne = rewriter.create<ConstantIndexOp>(parallelOp.getLoc(), 1);

 
 void mlir::populateParallelLoopToGPUPatterns(RewritePatternSet &patterns) {
   patterns.add<ParallelToGpuLaunchLowering>(patterns.getContext());
 }
 
 void mlir::configureParallelLoopToGPULegality(ConversionTarget &target) {
   target.addLegalDialect<memref::MemRefDialect>();
   target.addDynamicallyLegalOp<scf::ParallelOp>([](scf::ParallelOp parallelOp) {
-    return !parallelOp->getAttr(gpu::getMappingAttrName());
+    return !parallelOp->getAttr(gpu::getMappingAttrName()) ||
+           parallelOp->getAttr(gpu::getVisitedAttrName());
+  });
+}
+
+void mlir::finalizeParallelLoopToGPUConversion(Operation *op) {
+  op->walk([](scf::ParallelOp parallelOp) {
+    parallelOp->removeAttr(gpu::getVisitedAttrName());
   });
 }

mlir/lib/Conversion/SCFToGPU/SCFToGPUPass.cpp

void runOnOperation() override {
     RewritePatternSet patterns(&getContext());
     populateParallelLoopToGPUPatterns(patterns);
     ConversionTarget target(getContext());
     target.markUnknownOpDynamicallyLegal([](Operation *) { return true; });
     configureParallelLoopToGPULegality(target);
     if (failed(applyPartialConversion(getOperation(), target,
                                       std::move(patterns))))
       signalPassFailure();
+    finalizeParallelLoopToGPUConversion(getOperation());
   }
 };
 
 } // namespace
 
 std::unique_ptr<OperationPass<FuncOp>>
 mlir::createAffineForToGPUPass(unsigned numBlockDims, unsigned numThreadDims) {
   return std::make_unique<ForLoopMapper>(numBlockDims, numThreadDims);
 }
 std::unique_ptr<OperationPass<FuncOp>> mlir::createAffineForToGPUPass() {
   return std::make_unique<ForLoopMapper>();
 }
 
 std::unique_ptr<Pass> mlir::createParallelLoopToGpuPass() {
   return std::make_unique<ParallelLoopToGpuPass>();
 }

mlir/lib/Dialect/GPU/Transforms/ParallelLoopMapper.cpp

 using namespace mlir::scf;
 
 #include "mlir/Dialect/GPU/ParallelLoopMapperAttr.cpp.inc"
 #include "mlir/Dialect/GPU/ParallelLoopMapperEnums.cpp.inc"
 namespace mlir {
 namespace gpu {
 
 StringRef getMappingAttrName() { return "mapping"; }
+StringRef getVisitedAttrName() { return "GPU._visited_"; }
 
 ParallelLoopDimMapping getParallelLoopDimMappingAttr(Processor processor,
                                                      AffineMap map,
                                                      AffineMap bound) {
   MLIRContext *context = map.getContext();
   OpBuilder builder(context);
   return ParallelLoopDimMapping::get(
       ProcessorAttr::get(builder.getContext(), processor),

mlir/lib/Dialect/Tensor/Transforms/Bufferize.cpp

void runOnFunction() override {
     populateBufferizeMaterializationLegality(target);
 
     populateTensorBufferizePatterns(typeConverter, patterns);
     target.addIllegalOp<tensor::CastOp, tensor::ExtractOp,
                         tensor::FromElementsOp, tensor::GenerateOp>();
     target.addLegalDialect<memref::MemRefDialect>();
     target.addDynamicallyLegalDialect<StandardOpsDialect>(
         [&](Operation *op) { return typeConverter.isLegal(op); });
+    target.addLegalOp<ReturnOp>();
     target.addLegalDialect<scf::SCFDialect>();
 
     if (failed(
             applyPartialConversion(getFunction(), target, std::move(patterns))))
       signalPassFailure();
   }
 };
 } // namespace
 
 std::unique_ptr<Pass> mlir::createTensorBufferizePass() {
   return std::make_unique<TensorBufferizePass>();
 }

mlir/lib/Transforms/Utils/DialectConversion.cpp

 class OperationLegalizer {
 public:
   using LegalizationAction = ConversionTarget::LegalizationAction;
 
   OperationLegalizer(ConversionTarget &targetInfo,
                      const FrozenRewritePatternSet &patterns);
 
   /// Returns true if the given operation is known to be illegal on the target.
-  bool isIllegal(Operation *op) const;
+  bool isKnownIllegal(Operation *op) const;
 
   /// Attempt to legalize the given operation. Returns success if the operation
   /// was legalized, failure otherwise.
   LogicalResult legalize(Operation *op, ConversionPatternRewriter &rewriter);
 
   /// Returns the conversion target in use by the legalizer.
   ConversionTarget &getTarget() { return target; }
 
OperationLegalizer::OperationLegalizer(ConversionTarget &targetInfo,
   // them into legal ones.
   DenseMap<OperationName, LegalizationPatterns> legalizerPatterns;
   LegalizationPatterns anyOpLegalizerPatterns;
 
   buildLegalizationGraph(anyOpLegalizerPatterns, legalizerPatterns);
   computeLegalizationGraphBenefit(anyOpLegalizerPatterns, legalizerPatterns);
 }
 
-bool OperationLegalizer::isIllegal(Operation *op) const {
+bool OperationLegalizer::isKnownIllegal(Operation *op) const {
   // Check if the target explicitly marked this operation as illegal.
-  return target.getOpAction(op->getName()) == LegalizationAction::Illegal;
+  if (auto info = target.getOpAction(op->getName())) {
+    if (*info == LegalizationAction::Dynamic)
+      return !target.isLegal(op);
+    return *info == LegalizationAction::Illegal;
+  }
+
+  return false;
 }
 
 LogicalResult
 OperationLegalizer::legalize(Operation *op,
                              ConversionPatternRewriter &rewriter) {
 #ifndef NDEBUG
   const char *logLineComment =
       "//===-------------------------------------------===//\n";
if (failed(opLegalizer.legalize(op, rewriter))) {
     // Full conversions expect all operations to be converted.
     if (mode == OpConversionMode::Full)
       return op->emitError()
              << "failed to legalize operation '" << op->getName() << "'";
     // Partial conversions allow conversions to fail iff the operation was not
     // explicitly marked as illegal. If the user provided a nonlegalizableOps
     // set, non-legalizable ops are included.
     if (mode == OpConversionMode::Partial) {
-      if (opLegalizer.isIllegal(op))
+      if (opLegalizer.isKnownIllegal(op))
         return op->emitError()
                << "failed to legalize operation '" << op->getName()
                << "' that was explicitly marked illegal";
       if (trackedOps)
         trackedOps->insert(op);
     }
   } else if (mode == OpConversionMode::Analysis) {
     // Analysis conversions don't fail if any operations fail to legalize,

mlir/test/Transforms/test-legalizer-full.mlir

func @dynamic_func(%arg: i64) attributes {test.recursively_legal} {
     "test.return"() : () -> ()
   }
 
   "test.return"() : () -> ()
 }
 
 // -----
 
+// expected-remark@+1 {{applyFullConversion failed}}
+builtin.module {
+
 // Test that region cloning can be properly undone.
 func @test_undo_region_clone() {
   "test.region"() ({
     ^bb1(%i0: i64):
       "test.invalid"(%i0) : (i64) -> ()
   }) {legalizer.should_clone} : () -> ()
 
   // expected-error@+1 {{failed to legalize operation 'test.illegal_op_f'}}
   %ignored = "test.illegal_op_f"() : () -> (i32)
   "test.return"() : () -> ()
 }
 
+}
+
 // -----
 
+// expected-remark@+1 {{applyFullConversion failed}}
+builtin.module {
+
 // Test that unknown operations can be dynamically legal.
 func @test_unknown_dynamically_legal() {
   "foo.unknown_op"() {test.dynamically_legal} : () -> ()
 
   // expected-error@+1 {{failed to legalize operation 'foo.unknown_op'}}
   "foo.unknown_op"() {} : () -> ()
   "test.return"() : () -> ()
 }
 
+}
+
 // -----
 
+// expected-remark@+1 {{applyFullConversion failed}}
+builtin.module {
+
 // Test that region inlining can be properly undone.
 func @test_undo_region_inline() {
   "test.region"() ({
     ^bb1(%i0: i64):
        // expected-error@+1 {{failed to legalize operation 'std.br'}}
        br ^bb2(%i0 : i64)
     ^bb2(%i1: i64):
       "test.invalid"(%i1) : (i64) -> ()
   }) {} : () -> ()
 
   "test.return"() : () -> ()
 }
 
+}
+
 // -----
 
+// expected-remark@+1 {{applyFullConversion failed}}
+builtin.module {
+
 // Test that multiple block erases can be properly undone.
 func @test_undo_block_erase() {
    // expected-error@+1 {{failed to legalize operation 'test.region'}}
   "test.region"() ({
     ^bb1(%i0: i64):
        br ^bb2(%i0 : i64)
     ^bb2(%i1: i64):
       "test.invalid"(%i1) : (i64) -> ()
   }) {legalizer.should_clone, legalizer.erase_old_blocks} : () -> ()
 
   "test.return"() : () -> ()
 }
+
+}
+
+// -----
+
+// expected-remark@+1 {{applyFullConversion failed}}
+builtin.module {
+
+func @create_unregistered_op_in_pattern() -> i32 {
+  // expected-error@+1 {{failed to legalize operation 'test.illegal_op_g'}}
+  %0 = "test.illegal_op_g"() : () -> (i32)
+  "test.return"(%0) : (i32) -> ()
+}
+
+}

mlir/test/Transforms/test-legalizer.mlir

func @bounded_recursion() {
   // CHECK: test.recursive_rewrite 0
   test.recursive_rewrite 3
   // expected-remark@+1 {{op 'std.return' is not legalizable}}
   return
 }
 
 // -----
 
+// expected-remark@+1 {{applyPartialConversion failed}}
+builtin.module {
+
 func @fail_to_convert_illegal_op() -> i32 {
   // expected-error@+1 {{failed to legalize operation 'test.illegal_op_f'}}
   %result = "test.illegal_op_f"() : () -> (i32)
   return %result : i32
 }
 
+}
+
 // -----
 
+// expected-remark@+1 {{applyPartialConversion failed}}
+builtin.module {
+
 func @fail_to_convert_illegal_op_in_region() {
   // expected-error@+1 {{failed to legalize operation 'test.region_builder'}}
   "test.region_builder"() : () -> ()
   return
 }
 
+}
+
 // -----
 
 // Check that the entry block arguments of a region are untouched in the case
 // of failure.
 
-// CHECK-LABEL: func @fail_to_convert_region
+// expected-remark@+1 {{applyPartialConversion failed}}
+builtin.module {
+
 func @fail_to_convert_region() {
-  // CHECK-NEXT: "test.region"
+  // CHECK: "test.region"
   // CHECK-NEXT: ^bb{{.*}}(%{{.*}}: i64):
   "test.region"() ({
     ^bb1(%i0: i64):
       // expected-error@+1 {{failed to legalize operation 'test.region_builder'}}
       "test.region_builder"() : () -> ()
       "test.valid"() : () -> ()
   }) : () -> ()
   return
 }
 
+}
+
 // -----
 
 // CHECK-LABEL: @create_illegal_block
 func @create_illegal_block() {
   // Check that we can undo block creation, i.e. that the block was removed.
   // CHECK: test.create_illegal_block
   // CHECK-NOT: ^{{.*}}(%{{.*}}: i32, %{{.*}}: i32):
   // expected-remark@+1 {{op 'test.create_illegal_block' is not legalizable}}

 // CHECK-LABEL: @undo_child_created_before_parent
 func @undo_child_created_before_parent() {
   // expected-remark@+1 {{is not legalizable}}
   "test.illegal_op_with_region_anchor"() : () -> ()
   // expected-remark@+1 {{op 'std.return' is not legalizable}}
   return
 }
 
-
 // -----
 
-
 // Check that a conversion pattern on `test.blackhole` can mark the producer
 // for deletion.
 // CHECK-LABEL: @blackhole
 func @blackhole() {
   %input = "test.blackhole_producer"() : () -> (i32)
   "test.blackhole"(%input) : (i32) -> ()
   // expected-remark@+1 {{op 'std.return' is not legalizable}}
   return
 }
+
+// -----
+
+// expected-remark@+1 {{applyPartialConversion failed}}
+builtin.module {
+
+func @create_unregistered_op_in_pattern() -> i32 {
+  // expected-error@+1 {{failed to legalize operation 'test.illegal_op_g'}}
+  %0 = "test.illegal_op_g"() : () -> (i32)
+  "test.return"(%0) : (i32) -> ()
+}
+
+}

mlir/test/lib/Dialect/Test/TestOps.td

def Test_LegalizerEnum : StrEnumAttr<"Success", "Failure",
   [Test_LegalizerEnum_Success, Test_LegalizerEnum_Failure]>;
 
 def ILLegalOpA : TEST_Op<"illegal_op_a">, Results<(outs I32)>;
 def ILLegalOpB : TEST_Op<"illegal_op_b">, Results<(outs I32)>;
 def ILLegalOpC : TEST_Op<"illegal_op_c">, Results<(outs I32)>;
 def ILLegalOpD : TEST_Op<"illegal_op_d">, Results<(outs I32)>;
 def ILLegalOpE : TEST_Op<"illegal_op_e">, Results<(outs I32)>;
 def ILLegalOpF : TEST_Op<"illegal_op_f">, Results<(outs I32)>;
+def ILLegalOpG : TEST_Op<"illegal_op_g">, Results<(outs I32)>;
 def LegalOpA : TEST_Op<"legal_op_a">,
   Arguments<(ins Test_LegalizerEnum:$status)>, Results<(outs I32)>;
 def LegalOpB : TEST_Op<"legal_op_b">, Results<(outs I32)>;
+def LegalOpC : TEST_Op<"legal_op_c">,
+  Arguments<(ins I32)>, Results<(outs I32)>;
+def UnregisteredOp : TEST_Op<"unregistered_op">, Results<(outs I32)>;
 
 // Check that the conversion infrastructure can properly undo the creation of
 // operations where an operation was created before its parent, in this case,
 // in the parent's builder.
 def IllegalOpTerminator : TEST_Op<"illegal_op_terminator", [Terminator]>;
 def IllegalOpWithRegion : TEST_Op<"illegal_op_with_region"> {
   let skipDefaultBuilders = 1;
   let builders = [OpBuilder<(ins),

mlir/test/lib/Dialect/Test/TestPatterns.cpp

LogicalResult matchAndRewrite(BlackHoleOp op,
     Operation *producer = op.getOperand().getDefiningOp();
     // Always erase the user before the producer, the framework should handle
     // this correctly.
     rewriter.eraseOp(op);
     rewriter.eraseOp(producer);
     return success();
   };
 };
+
+// This pattern replaces explicitly illegal op with explicitly legal op,
+// but in addition creates unregistered operation.
+struct TestCreateUnregisteredOp : public OpRewritePattern<ILLegalOpG> {
+  using OpRewritePattern<ILLegalOpG>::OpRewritePattern;
+
+  LogicalResult matchAndRewrite(ILLegalOpG op,
+                                PatternRewriter &rewriter) const final {
+    Value val = rewriter.create<UnregisteredOp>(op->getLoc());
+    rewriter.replaceOpWithNewOp<LegalOpC>(op, val);
+    return success();
+  };
+};
 } // namespace
 
 namespace {
 struct TestTypeConverter : public TypeConverter {
   using TypeConverter::TypeConverter;
   TestTypeConverter() {
     addConversion(convertType);
     addArgumentMaterialization(materializeCast);
void runOnOperation() override {
     populateWithGenerated(patterns);
     patterns
         .add<TestRegionRewriteBlockMovement, TestRegionRewriteUndo,
              TestCreateBlock, TestCreateIllegalBlock, TestUndoBlockArgReplace,
              TestUndoBlockErase, TestPassthroughInvalidOp, TestSplitReturnType,
              TestChangeProducerTypeI32ToF32, TestChangeProducerTypeF32ToF64,
              TestChangeProducerTypeF32ToInvalid, TestUpdateConsumerType,
              TestNonRootReplacement, TestBoundedRecursiveRewrite,
-             TestNestedOpCreationUndoRewrite, TestReplaceEraseOp>(
-            &getContext());
+             TestNestedOpCreationUndoRewrite, TestReplaceEraseOp,
+             TestCreateUnregisteredOp>(&getContext());
     patterns.add<TestDropOpSignatureConversion>(&getContext(), converter);
     mlir::populateFuncOpTypeConversionPattern(patterns, converter);
     mlir::populateCallOpTypeConversionPattern(patterns, converter);
 
     // Define the conversion target used for the test.
     ConversionTarget target(getContext());
     target.addLegalOp<ModuleOp>();
-    target.addLegalOp<LegalOpA, LegalOpB, TestCastOp, TestValidOp,
+    target.addLegalOp<LegalOpA, LegalOpB, LegalOpC, TestCastOp, TestValidOp,
                       TerminatorOp>();
     target
         .addIllegalOp<ILLegalOpF, TestRegionBuilderOp, TestOpWithRegionFold>();
     target.addDynamicallyLegalOp<TestReturnOp>([](TestReturnOp op) {
       // Don't allow F32 operands.
       return llvm::none_of(op.getOperandTypes(),
                            [](Type type) { return type.isF32(); });
     });
     target.addDynamicallyLegalOp<FuncOp>([&](FuncOp op) {
       return converter.isSignatureLegal(op.getType()) &&
              converter.isLegal(&op.getBody());
     });
+    target.addDynamicallyLegalOp<ILLegalOpG>([](ILLegalOpG) { return false; });
 
     // Expect the type_producer/type_consumer operations to only operate on f64.
     target.addDynamicallyLegalOp<TestTypeProducerOp>(
         [](TestTypeProducerOp op) { return op.getType().isF64(); });
     target.addDynamicallyLegalOp<TestTypeConsumerOp>([](TestTypeConsumerOp op) {
       return op.getOperand().getType().isF64();
     });
 
     // Check support for marking certain operations as recursively legal.
     target.markOpRecursivelyLegal<FuncOp, ModuleOp>([](Operation *op) {
       return static_cast<bool>(
           op->getAttrOfType<UnitAttr>("test.recursively_legal"));
     });
 
     // Mark the bound recursion operation as dynamically legal.
     target.addDynamicallyLegalOp<TestRecursiveRewriteOp>(
         [](TestRecursiveRewriteOp op) { return op.depth() == 0; });
 
     // Handle a partial conversion.
     if (mode == ConversionMode::Partial) {
       DenseSet<Operation *> unlegalizedOps;
-      (void)applyPartialConversion(getOperation(), target, std::move(patterns),
-                                   &unlegalizedOps);
+      if (failed(applyPartialConversion(
+              getOperation(), target, std::move(patterns), &unlegalizedOps))) {
+        getOperation()->emitRemark() << "applyPartialConversion failed";
+      }
       // Emit remarks for each legalizable operation.
       for (auto *op : unlegalizedOps)
         op->emitRemark() << "op '" << op->getName() << "' is not legalizable";
       return;
     }
 
     // Handle a full conversion.
     if (mode == ConversionMode::Full) {
       // Check support for marking unknown operations as dynamically legal.
       target.markUnknownOpDynamicallyLegal([](Operation *op) {
         return (bool)op->getAttrOfType<UnitAttr>("test.dynamically_legal");
       });
 
-      (void)applyFullConversion(getOperation(), target, std::move(patterns));
+      if (failed(applyFullConversion(getOperation(), target,
+                                     std::move(patterns)))) {
+        getOperation()->emitRemark() << "applyFullConversion failed";
+      }
       return;
     }
 
     // Otherwise, handle an analysis conversion.
     assert(mode == ConversionMode::Analysis);
 
     // Analyze the convertible operations.
     DenseSet<Operation *> legalizedOps;