[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

57

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

38

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

57

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.

[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

 //===- OpenACCToSCF.cpp - OpenACC condition to SCF if conversion ----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "mlir/Conversion/OpenACCToSCF/ConvertOpenACCToSCF.h"
 
 #include "mlir/Dialect/Arith/IR/Arith.h"
 #include "mlir/Dialect/OpenACC/OpenACC.h"
 #include "mlir/Dialect/SCF/IR/SCF.h"
+#include "mlir/IR/Matchers.h"
 #include "mlir/Pass/Pass.h"
 #include "mlir/Transforms/DialectConversion.h"
 
 namespace mlir {
 #define GEN_PASS_DEF_CONVERTOPENACCTOSCF
 #include "mlir/Conversion/Passes.h.inc"
 } // namespace mlir
 
 using namespace mlir;
 
 //===----------------------------------------------------------------------===//
 // Conversion patterns
 //===----------------------------------------------------------------------===//
 
 namespace {
 /// 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();
+      return failure();
 
-    // Condition is not a constant.
-    if (!op.getIfCond().template getDefiningOp<arith::ConstantOp>()) {
+    IntegerAttr constAttr;
+    if (!matchPattern(op.getIfCond(), m_Constant(&constAttr))) {
       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);
+    } else {
+      if (constAttr.getInt())
+        rewriter.updateRootInPlace(op,
+                                   [&]() { op.getIfCondMutable().erase(0); });
+      else
+        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());

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
+
+// -----