[mlir] add RemoveConstantIfCondition to populateOpenACCToSCFConversionPatterns

[python3kgae]

Summary

For issue https://github.com/llvm/llvm-project/issues/60058
It hit assert when legalizePatternResult on success of ExpandIfCondition which did nothing just return success when if condition is constant.

Added RemoveConstantIfCondition to remove the if cond by getCanonicalizationPatterns.
Also remove the check for constant if cond in ExpandIfCondition and
change check ifCond to assert because only op with ifCond will need legalize in ConvertOpenACCToSCFPass

[mehdi_amini]

Nit: if you refer to a GitHub issue as Fixes #60058 in the commit message, it'll automatically close the issue when you push.

[clementval]

Why creating an op if we know we don't need it?

[clementval]

mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp

38

ifCond is an optional so if it's not present we should do nothing. The assert is not checking the same things. If the op has no ifCond it will just not compile.

[clementval]

mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp

37

This will probably trigger the assert when ifCond is absent.

[python3kgae]

In D142286#4071215, @clementval wrote:

Why creating an op if we know we don't need it?

ExpandIfCondition should not cause crash with or without RemoveConstantIfCondition.
When RemoveConstantIfCondition is not there, return success but do nothing will cause assert fail in OperationLegalizer::legalizePatternResult.

assert((replacedRoot() || updatedRootInPlace()) &&
       "expected pattern to replace the root operation");

mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp

37

It is controlled by

target.addDynamicallyLegalOp<acc::ExitDataOp>(
    [](acc::ExitDataOp op) { return !op.getIfCond(); });

in
https://github.com/llvm/llvm-project/blob/main/mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp#L85

Only Ops which has ifCond need to be legalized.

38

It is optional, but ExpandIfCondition only hit on Ops with ifCond.

[mehdi_amini]

Could just inverse the condition and return failure()?

[python3kgae]

In D142286#4071254, @mehdi_amini wrote:

Could just inverse the condition and return failure()?

Not sure which path you're talking about :(

change assert(op.getIfCond() && "expected Op with IfCond"); to

if (!op.getIfCond())
  return failure();

or
add back the check ifCond as constant like
if (op.getIfCond().template getDefiningOp<arith::ConstantOp>())

return failure();

Or both?

[python3kgae]

In D142286#4071304, @python3kgae wrote:

In D142286#4071254, @mehdi_amini wrote:

Could just inverse the condition and return failure()?

Not sure which path you're talking about :(

change assert(op.getIfCond() && "expected Op with IfCond"); to

if (!op.getIfCond())
  return failure();

or
add back the check ifCond as constant like
if (op.getIfCond().template getDefiningOp<arith::ConstantOp>())

return failure();

Or both?

For if (op.getIfCond().template getDefiningOp<arith::ConstantOp>())
maybe we could just copy code of RemoveConstantIfCondition instead of return failure() and remove

 acc::EnterDataOp::getCanonicalizationPatterns(patterns,
                                              patterns.getContext());
acc::ExitDataOp::getCanonicalizationPatterns(patterns, patterns.getContext());
acc::UpdateOp::getCanonicalizationPatterns(patterns, patterns.getContext());

in current change?

[mehdi_amini]

In D142286#4071304, @python3kgae wrote:

In D142286#4071254, @mehdi_amini wrote:

Could just inverse the condition and return failure()?

Not sure which path you're talking about :(

change assert(op.getIfCond() && "expected Op with IfCond"); to

if (!op.getIfCond())
  return failure();

Yeah I wonder if this wouldn't just be enough for the crash here?

if (op.getIfCond().template getDefiningOp<arith::ConstantOp>())

return failure();

Or both?

This is also a possibility to have both, I assume that we didn't want to build an if that would be folded away?

[python3kgae]

Do RemoveConstantIfCondition in ExpandIfCondition for constant if cond.

[clementval]

LGTM

mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp

 /// Pattern to transform the `getIfCond` on operation without region into a
 /// scf.if and move the operation into the `then` region.
 template <typename OpTy>
 class ExpandIfCondition : public OpRewritePattern<OpTy> {
   using OpRewritePattern<OpTy>::OpRewritePattern;
 
   LogicalResult matchAndRewrite(OpTy op,
                                 PatternRewriter &rewriter) const override {
-    // Early exit if there is no condition.
-    if (!op.getIfCond())
-      return success();
+    assert(op.getIfCond() && "expected Op with IfCond");
 
-    // Condition is not a constant.
-    if (!op.getIfCond().template getDefiningOp<arith::ConstantOp>()) {
-      auto ifOp = rewriter.create<scf::IfOp>(op.getLoc(), TypeRange(),
-                                             op.getIfCond(), false);
-      rewriter.updateRootInPlace(op, [&]() { op.getIfCondMutable().erase(0); });
-      auto thenBodyBuilder = ifOp.getThenBodyBuilder();
-      thenBodyBuilder.setListener(rewriter.getListener());
-      thenBodyBuilder.clone(*op.getOperation());
-      rewriter.eraseOp(op);
-    }
 
     return success();
   }
 };
 } // namespace
 
 void mlir::populateOpenACCToSCFConversionPatterns(RewritePatternSet &patterns) {
   patterns.add<ExpandIfCondition<acc::EnterDataOp>>(patterns.getContext());
   patterns.add<ExpandIfCondition<acc::ExitDataOp>>(patterns.getContext());
   patterns.add<ExpandIfCondition<acc::UpdateOp>>(patterns.getContext());
+  acc::EnterDataOp::getCanonicalizationPatterns(patterns,
+                                                patterns.getContext());
+  acc::ExitDataOp::getCanonicalizationPatterns(patterns, patterns.getContext());
+  acc::UpdateOp::getCanonicalizationPatterns(patterns, patterns.getContext());
 }
 
 namespace {
 struct ConvertOpenACCToSCFPass
     : public impl::ConvertOpenACCToSCFBase<ConvertOpenACCToSCFPass> {
   void runOnOperation() override;
 };
 } // namespace

mlir/test/Conversion/OpenACCToSCF/convert-openacc-to-scf.mlir

func.func @testupdateop(%a: memref<10xf32>, %ifCond: i1) -> () {
   acc.update if(%ifCond) host(%a: memref<10xf32>)
   return
 }
 
 // CHECK:      func @testupdateop(%{{.*}}: memref<10xf32>, [[IFCOND:%.*]]: i1)
 // CHECK:        scf.if [[IFCOND]] {
 // CHECK-NEXT:     acc.update host(%{{.*}} : memref<10xf32>)
 // CHECK-NEXT:   }
+
+// -----
+
+func.func @update_true(%arg0: memref<10xf32, #spirv.storage_class<StorageBuffer>>) {
+  %true = arith.constant true
+  acc.update if(%true) host(%arg0 : memref<10xf32, #spirv.storage_class<StorageBuffer>>)
+  return
+}
+
+// CHECK-LABEL: func.func @update_true
+// CHECK-NOT:if
+// CHECK:acc.update host
+
+// -----
+
+func.func @update_false(%arg0: memref<10xf32, #spirv.storage_class<StorageBuffer>>) {
+  %false = arith.constant false
+  acc.update if(%false) host(%arg0 : memref<10xf32, #spirv.storage_class<StorageBuffer>>)
+  return
+}
+
+// CHECK-LABEL: func.func @update_false
+// CHECK-NOT:acc.update
+
+// -----
+
+func.func @enter_data_true(%d1 : memref<10xf32>) {
+  %true = arith.constant true
+  acc.enter_data if(%true) create(%d1 : memref<10xf32>) attributes {async}
+  return
+}
+
+// CHECK-LABEL: func.func @enter_data_true
+// CHECK-NOT:if
+// CHECK:acc.enter_data create
+
+// -----
+
+func.func @enter_data_false(%d1 : memref<10xf32>) {
+  %false = arith.constant false
+  acc.enter_data if(%false) create(%d1 : memref<10xf32>) attributes {async}
+  return
+}
+
+// CHECK-LABEL: func.func @enter_data_false
+// CHECK-NOT:acc.enter_data
+
+// -----
+
+func.func @exit_data_true(%d1 : memref<10xf32>) {
+  %true = arith.constant true
+  acc.exit_data if(%true)  delete(%d1 : memref<10xf32>) attributes {async}
+  return
+}
+
+// CHECK-LABEL: func.func @exit_data_true
+// CHECK-NOT:if
+// CHECK:acc.exit_data delete
+
+// -----
+
+func.func @exit_data_false(%d1 : memref<10xf32>) {
+  %false = arith.constant false
+  acc.exit_data if(%false)  delete(%d1 : memref<10xf32>) attributes {async}
+  return
+}
+
+// CHECK-LABEL: func.func @exit_data_false
+// CHECK-NOT:acc.exit_data
+
+// -----