[mlir] allow empty region in transform::SequenceOp::getEffects

[python3kgae]

Summary

Fixes #60213 https://github.com/llvm/llvm-project/issues/60213
When region is empty, getBodyBlock should return nullptr.
And transform::SequenceOp::getEffects should not use the nullptr return from getBodyBlock.

[ftynse]

mlir/test/IR/visitors.mlir

214

I don't this a change to the Transform dialect should be affecting test/IR/, which is really for core IR features. I understand this is how the buggy behavior was exposed originally, but we need to find a more suitable place. One trivial solution would be to just have this part of test under test/Dialect/Transform. We may also want to specifically exercise the memory effects rather than the visitor that feels a bit orthogonal.

[python3kgae]

mlir/test/IR/visitors.mlir

214

Is it OK to add an unit test for getBodyBlock on empty region to return nullptr?

And I saw many calls on getBodyBlock which assume it not return nullptr, shall I add check for all the calls of getBodyBlock?

mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h

 public:
   /// Verifies that `op` satisfies the invariants of this trait. Not expected to
   /// be called directly.
   static LogicalResult verifyTrait(Operation *op) {
     return detail::verifyPossibleTopLevelTransformOpTrait(op);
   }
 
   /// Returns the single block of the given region.
-  Block *getBodyBlock(unsigned region = 0) {
-    return &this->getOperation()->getRegion(region).front();
-  }
+ Block *getBodyBlock(unsigned regionIndex = 0) {
+   auto &region = this->getOperation()->getRegion(regionIndex);
+   return region.empty() ? nullptr : &region.front();
 
   /// Sets up the mapping between the entry block of the given region of this op
   /// and the relevant list of Payload IR operations in the given state. The
   /// state is expected to be already scoped at the region of this operation.
   LogicalResult mapBlockArguments(TransformState &state, Region &region) {
     assert(region.getParentOp() == this->getOperation() &&
            "op comes from the wrong region");
     return detail::mapPossibleTopLevelTransformOpBlockArguments(

mlir/lib/Dialect/Transform/IR/TransformOps.cpp

for (Operation &op : *getBodyBlock()) {
       }
 
       SmallVector<MemoryEffects::EffectInstance, 2> nestedEffects;
       iface.getEffects(effects);
     }
     return;
   }
 
+  // Early exit on empty region.
+  if (!getBodyBlock())
+    return;
+
   // Carry over all effects on the argument of the entry block as those on the
   // operand, this is the same value just remapped.
   for (Operation &op : *getBodyBlock()) {
     auto iface = dyn_cast<MemoryEffectOpInterface>(&op);
     if (!iface) {
       // TODO: fill all possible effects; or require ops to actually implement
       // the memory effect interface always
       assert(false);

mlir/test/IR/visitors.mlir

 // CHECK:       Erasing op 'func.return'
 
 // CHECK-LABEL: Block post-order erasures (no skip)
 // CHECK:       Erasing block ^bb0 from region 0 from operation 'regionOp0'
 // CHECK:       Erasing block ^bb0 from region 0 from operation 'regionOp0'
 // CHECK:       Erasing block ^bb0 from region 0 from operation 'regionOp0'
 // CHECK:       Erasing block ^bb0 from region 0 from operation 'func.func'
 // CHECK:       Erasing block ^bb0 from region 0 from operation 'builtin.module'
+
+// -----
+
+transform.sequence failures(propagate) {
+^bb0(%arg0: !pdl.operation):
+  sequence %arg0 : !pdl.operation failures(propagate) {
+  ^bb0(%arg1: !pdl.operation):  
+  }  
+}
+
+// CHECK-LABEL: Op pre-order visits
+// CHECK: Visiting op 'builtin.module'
+// CHECK: Visiting op 'transform.sequence'
+// CHECK: Visiting op 'transform.sequence'
+// CHECK: Visiting op 'transform.yield'
+// CHECK: Visiting op 'transform.yield'
+// CHECK-LABEL:Block pre-order visits
+// CHECK: Visiting block ^bb0 from region 0 from operation 'builtin.module'
+// CHECK: Visiting block ^bb0 from region 0 from operation 'transform.sequence'
+// CHECK: Visiting block ^bb0 from region 0 from operation 'transform.sequence'
+// CHECK-LABEL:Region pre-order visits
+// CHECK: Visiting region 0 from operation 'builtin.module'
+// CHECK: Visiting region 0 from operation 'transform.sequence'
+// CHECK: Visiting region 0 from operation 'transform.sequence'
+// CHECK-LABEL:Op post-order visits
+// CHECK: Visiting op 'transform.yield'
+// CHECK: Visiting op 'transform.sequence'
+// CHECK: Visiting op 'transform.yield'
+// CHECK: Visiting op 'transform.sequence'
+// CHECK: Visiting op 'builtin.module'
+// CHECK-LABEL:Block post-order visits
+// CHECK: Visiting block ^bb0 from region 0 from operation 'transform.sequence'
+// CHECK: Visiting block ^bb0 from region 0 from operation 'transform.sequence'
+// CHECK: Visiting block ^bb0 from region 0 from operation 'builtin.module'
+// CHECK-LABEL:Region post-order visits
+// CHECK: Visiting region 0 from operation 'transform.sequence'
+// CHECK: Visiting region 0 from operation 'transform.sequence'
+// CHECK: Visiting region 0 from operation 'builtin.module'
+// CHECK-LABEL:Op pre-order erasures (skip)
+// CHECK: Erasing op 'transform.sequence'
+// CHECK: Erasing op 'transform.yield'
+// CHECK-LABEL:Block pre-order erasures (skip)
+// CHECK: Erasing block ^bb0 from region 0 from operation 'transform.sequence'
+// CHECK-LABEL:Op post-order erasures (skip)
+// CHECK: Erasing op 'transform.yield'
+// CHECK: Erasing op 'transform.sequence'
+// CHECK: Erasing op 'transform.yield'
+// CHECK-LABEL:Block post-order erasures (skip)
+// CHECK: Erasing block ^bb0 from region 0 from operation 'transform.sequence'
+// CHECK-LABEL:Op post-order erasures (no skip)
+// CHECK: Erasing op 'transform.yield'
+// CHECK: Erasing op 'transform.sequence'
+// CHECK: Erasing op 'transform.yield'
+// CHECK: Erasing op 'transform.sequence'
+// CHECK: Erasing op 'builtin.module'
+// CHECK-LABEL:Block post-order erasures (no skip)
+// CHECK: Erasing block ^bb0 from region 0 from operation 'transform.sequence'
+// CHECK: Erasing block ^bb0 from region 0 from operation 'transform.sequence'
+// CHECK: Erasing block ^bb0 from region 0 from operation 'builtin.module'