[mlir][openacc] Add conversion for if operand to scf.if for standalone data operation

[clementval]

Summary

This patch convert the if condition on standalone data operation such as acc.update,
acc.enter_data and acc.exit_data to a scf.if with the operation in the if region.
It removes the operation when the if condition is constant and false. It removes the
the condition if it is contant and true.

Conversion to scf.if is done in order to use the translation to LLVM IR dialect out of the box.
Not sure this is the best approach or we should perform this during the translation from OpenACC
to LLVM IR dialect. Any thoughts welcome.

[schweitz]

LGTM.

[kiranchandramohan]

I have three points for discussion,

1. There is an SCF to OpenMP conversion, I can imagine an SCF to OpenACC conversion as well. This (OpenACC to SCF) would then create a cycle. Is that a problem?
2. Can this be a transformation pass?
3. For the OpenMP Parallel operation, the if clause is handled during translation. The CreateParallel function in the OpenMP IRBuilder accepts an if value and inserts an if-then-else. For consistency, should we do this during translation?

https://github.com/llvm/llvm-project/blob/2644399ce7721cba9a546a9af09fd2a942a4d0cd/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp#L531

[clementval]

In D103325#2787818, @kiranchandramohan wrote:

1. There is an SCF to OpenMP conversion, I can imagine an SCF to OpenACC conversion as well. This (OpenACC to SCF) would then create a cycle. Is that a problem?

You are right. I guess it depends how the final pipeline is set up. If the correct patterns are applied at the correct time it should work.

2. Can this be a transformation pass?

Yeah that's probably better than conversion since it does not really convert the op to another op.

3. For the OpenMP Parallel operation, the if clause is handled during translation. The CreateParallel function in the OpenMP IRBuilder accepts an if value and inserts an if-then-else. For consistency, should we do this during translation?

https://github.com/llvm/llvm-project/blob/2644399ce7721cba9a546a9af09fd2a942a4d0cd/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp#L531

I'm fine doing this in translation. The only point I see where it would be more beneficial to pass by SCF is for example if we want to translate to smth else than LLVM IR someday (e.g. SPIR-V). In that case going through scf would allow to use a single transformation to scf.if and reuse the available translation.

[clementval]

In D103325#2787818, @kiranchandramohan wrote:

2. Can this be a transformation pass?

@ftynse @rriddle Are there some best practices about conversion passes vs. transformation passes?

[clementval]

Fix header guard

[kiranchandramohan]

In D103325#2792690, @clementval wrote:

In D103325#2787818, @kiranchandramohan wrote:

2. Can this be a transformation pass?

@ftynse @rriddle Are there some best practices about conversion passes vs. transformation passes?

I think this is a good question to discuss in discourse. You can point to this review as well.

[clementval]

In D103325#2793944, @kiranchandramohan wrote:

In D103325#2792690, @clementval wrote:

In D103325#2787818, @kiranchandramohan wrote:

2. Can this be a transformation pass?

@ftynse @rriddle Are there some best practices about conversion passes vs. transformation passes?

I think this is a good question to discuss in discourse. You can point to this review as well.

Sure. Topic opened here; https://llvm.discourse.group/t/conversion-vs-transformation-pass/3567

[ftynse]

In D103325#2787818, @kiranchandramohan wrote:

I have three points for discussion,

1. There is an SCF to OpenMP conversion, I can imagine an SCF to OpenACC conversion as well. This (OpenACC to SCF) would then create a cycle. Is that a problem?

A priori, no. Passes are executed one by one and the pass infrastructure has no knowledge of what happens inside the pass, e.g. whether the pass is using the dialect conversion infrastructure or not so there is no chance of patterns mutually undoing each other effects.

2. Can this be a transformation pass?

The real reason why lib/Conversion was created is library layering. A pass that converts DialectA to DialectB depends on both dialects for obvious reasons. If we put it in either DialectA or in DialectB, we would create a dependency between dialects that is otherwise avoidable. So we put the pass in lib/Conversion/DialectAToDialectB. If DialectB already depends on DialectA, the pass can live within DialectB just fine because it is not introducing any new dependency.

3. For the OpenMP Parallel operation, the if clause is handled during translation. The CreateParallel function in the OpenMP IRBuilder accepts an if value and inserts an if-then-else. For consistency, should we do this during translation?

https://github.com/llvm/llvm-project/blob/2644399ce7721cba9a546a9af09fd2a942a4d0cd/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp#L531

Personally, I would prefer doing as many things as possible in MLIR and have a trivial translation. Most of the complexity currently in the translation infrastructure is only there to support the use of OpenMPIRBuilder. On the other hand, having two pieces of code that serve the same purpose within the larger LLVM infrastructure is also undesirable...

[ftynse]

mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp

23

Nit: please add documentation comments for top-level entries

25

Nit: you can put constexpr static kIfConditionOperandPos = 0 in extraClassDeclaration of the relevant op definitions, than use OpTy:kIfConditionOperandPos. This is future-proof should the position change and makes the code a bit more readable.

26–35

Mutating an operation using Operation:: API is invisible to PatternRewriter and may make the entire conversion crash. Wrap this into rewriter.updateRootInPlace().

27–36

Actually, given a concrete op type, it should be possible to do something like op.ifConditionMutable().erase(), which updates the attribute itself without all the magic numbers here.

64–65

This feels like it should be a separate pattern that is also run by the canonicalizer.

69–70

Why is this necessary? The condition has been removed from the op already...

[clementval]

Address review comments.

[clementval]

mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp

27–36

Thanks! This is what I was looking for. It's way cleaner than the magic numbers stuff and future-proof.

64–65

Move it has a canonicalizer in a separate patch (D103712). Do you think it should it be called here as well?

[clementval]

Remove changes that are not needed anymore.

[ftynse]

Please make the rewriter aware of the inplace mutation. Otherwise LGTM.

mlir/include/mlir/Conversion/OpenACCToSCF/ConvertOpenACCToSCF.h

23

I suppose this linter error means you need to #include <memory>

mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp

40

This is still an inplace mutation that needs rewriter.updateRootInPlace around it.

64–65

You can add the canonicalization pattern here as well if it help enable other patterns.

[clementval]

Address review comments

[ftynse]

mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp

41

Sorry, missed this one before, getThenBodyBuilder is unaware of the rewriter either. You need to either set the rewriter to the same location or call .setListener(rewriter.getListener()) on it before using.

[clementval]

Address review comments.

[clementval]

mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp

41

Thanks. Didn't know we had to do this here.

[ftynse]

mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp

41

Generally, anything IR modification needs to go through the rewriter if you have one. So any call that is not rewriter. needs to be communicated to the rewriter somehow.

[clementval]

mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp

41

Got it. Thanks!

mlir/include/mlir/Conversion/OpenACCToSCF/ConvertOpenACCToSCF.h

+//===- ConvertOpenACCToSCF.h - OpenACC conversion pass entrypoint ---------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+#ifndef MLIR_CONVERSION_OPENACCTOLLVM_CONVERTOPENACCTOSCF_H
+#define MLIR_CONVERSION_OPENACCTOLLVM_CONVERTOPENACCTOSCF_H
+
+namespace mlir {
+class ModuleOp;
+template <typename T>
+class OperationPass;
+class RewritePatternSet;
+
+/// Collect the patterns to convert from the OpenACC dialect to OpenACC with
+/// SCF dialect.
+void populateOpenACCToSCFConversionPatterns(RewritePatternSet &patterns);
+
+/// Create a pass to convert the OpenACC dialect into the LLVMIR dialect.
+std::unique_ptr<OperationPass<ModuleOp>> createConvertOpenACCToSCFPass();
+
+} // namespace mlir
+
+#endif // MLIR_CONVERSION_OPENACCTOLLVM_CONVERTOPENACCTOSCF_H

mlir/include/mlir/Conversion/Passes.h

 #include "mlir/Conversion/GPUToROCDL/GPUToROCDLPass.h"
 #include "mlir/Conversion/GPUToSPIRV/GPUToSPIRVPass.h"
 #include "mlir/Conversion/GPUToVulkan/ConvertGPUToVulkanPass.h"
 #include "mlir/Conversion/LinalgToLLVM/LinalgToLLVM.h"
 #include "mlir/Conversion/LinalgToSPIRV/LinalgToSPIRVPass.h"
 #include "mlir/Conversion/LinalgToStandard/LinalgToStandard.h"
 #include "mlir/Conversion/MathToLibm/MathToLibm.h"
 #include "mlir/Conversion/OpenACCToLLVM/ConvertOpenACCToLLVM.h"
+#include "mlir/Conversion/OpenACCToSCF/ConvertOpenACCToSCF.h"
 #include "mlir/Conversion/OpenMPToLLVM/ConvertOpenMPToLLVM.h"
 #include "mlir/Conversion/PDLToPDLInterp/PDLToPDLInterp.h"
 #include "mlir/Conversion/SCFToGPU/SCFToGPUPass.h"
 #include "mlir/Conversion/SCFToOpenMP/SCFToOpenMP.h"
 #include "mlir/Conversion/SCFToSPIRV/SCFToSPIRVPass.h"
 #include "mlir/Conversion/SCFToStandard/SCFToStandard.h"
 #include "mlir/Conversion/SPIRVToLLVM/SPIRVToLLVMPass.h"
 #include "mlir/Conversion/ShapeToStandard/ShapeToStandard.h"

mlir/include/mlir/Conversion/Passes.td

def ConvertMathToLibm : Pass<"convert-math-to-libm", "ModuleOp"> {
   let description = [{
     This pass converts supported Math ops to libm calls.
   }];
   let constructor = "mlir::createConvertMathToLibmPass()";
   let dependentDialects = ["StandardOpsDialect", "vector::VectorDialect"];
 }
 
 //===----------------------------------------------------------------------===//
+// OpenACCToSCF
+//===----------------------------------------------------------------------===//
+
+def ConvertOpenACCToSCF : Pass<"convert-openacc-to-scf", "ModuleOp"> {
+  let summary = "Convert the OpenACC ops to OpenACC with SCF dialect";
+  let constructor = "mlir::createConvertOpenACCToSCFPass()";
+  let dependentDialects = ["scf::SCFDialect", "acc::OpenACCDialect"];
+}
+
+//===----------------------------------------------------------------------===//
 // OpenACCToLLVM
 //===----------------------------------------------------------------------===//
 
 def ConvertOpenACCToLLVM : Pass<"convert-openacc-to-llvm", "ModuleOp"> {
   let summary = "Convert the OpenACC ops to LLVM dialect";
   let constructor = "mlir::createConvertOpenACCToLLVMPass()";
   let dependentDialects = ["LLVM::LLVMDialect"];
 }

mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td

let description = [{
 
     Example:
 
     ```mlir
     acc.update device(%d1 : memref<10xf32>) attributes {async}
     ```
   }];
 
-  let arguments = (ins Optional<IntOrIndex>:$asyncOperand,
+  let arguments = (ins Optional<I1>:$ifCond,
+                       Optional<IntOrIndex>:$asyncOperand,
                        Optional<IntOrIndex>:$waitDevnum,
                        Variadic<IntOrIndex>:$waitOperands,
                        UnitAttr:$async,
                        UnitAttr:$wait,
                        Variadic<IntOrIndex>:$deviceTypeOperands,
-                       Optional<I1>:$ifCond,
                        Variadic<AnyType>:$hostOperands,
                        Variadic<AnyType>:$deviceOperands,
                        UnitAttr:$ifPresent);
 
   let extraClassDeclaration = [{
     /// The number of data operands.
     unsigned getNumDataOperands();
 

mlir/lib/Conversion/CMakeLists.txt

 add_subdirectory(AffineToStandard)
 add_subdirectory(AsyncToLLVM)
 add_subdirectory(ComplexToLLVM)
 add_subdirectory(ComplexToStandard)
 add_subdirectory(GPUCommon)
 add_subdirectory(GPUToNVVM)
 add_subdirectory(GPUToROCDL)
 add_subdirectory(GPUToSPIRV)
 add_subdirectory(GPUToVulkan)
 add_subdirectory(LinalgToLLVM)
 add_subdirectory(LinalgToSPIRV)
 add_subdirectory(LinalgToStandard)
 add_subdirectory(MathToLibm)
 add_subdirectory(OpenACCToLLVM)
+add_subdirectory(OpenACCToSCF)
 add_subdirectory(OpenMPToLLVM)
 add_subdirectory(PDLToPDLInterp)
 add_subdirectory(SCFToGPU)
 add_subdirectory(SCFToOpenMP)
 add_subdirectory(SCFToSPIRV)
 add_subdirectory(SCFToStandard)
 add_subdirectory(ShapeToStandard)
 add_subdirectory(SPIRVToLLVM)

mlir/lib/Conversion/OpenACCToSCF/CMakeLists.txt

+add_mlir_conversion_library(MLIROpenACCToSCF
+  OpenACCToSCF.cpp
+
+  ADDITIONAL_HEADER_DIRS
+  ${MLIR_MAIN_INCLUDE_DIR}/mlir/Conversion/OpenACCToSCF
+
+  DEPENDS
+  MLIRConversionPassIncGen
+
+  LINK_LIBS PUBLIC
+  MLIRIR
+  MLIROpenACC
+  MLIRTransforms
+  MLIRSCF
+  )

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 "../PassDetail.h"
+#include "mlir/Conversion/OpenACCToSCF/ConvertOpenACCToSCF.h"
+#include "mlir/Dialect/OpenACC/OpenACC.h"
+#include "mlir/Dialect/SCF/SCF.h"
+#include "mlir/Dialect/StandardOps/IR/Ops.h"
+#include "mlir/Transforms/DialectConversion.h"
+
+using namespace mlir;
+
+//===----------------------------------------------------------------------===//
+// Utility functions
+//===----------------------------------------------------------------------===//
+
+template <typename OpTy>
+static void removeIfCondition(OpTy &op) {
+  // ifCondition is on position 0 for all standalone data operation.
+  op.getOperation()->eraseOperand(0);
+  auto attrName = op.getOperandSegmentSizeAttr();
+  auto sizeAttr =
+      op.getOperation()->template getAttrOfType<DenseIntElementsAttr>(attrName);
+  SmallVector<int32_t> sizes;
+  sizes.push_back(0); // remove ifCond in first position.
+  for (auto size : llvm::enumerate(sizeAttr.getIntValues()))
+    if (size.index() != 0)
+      sizes.push_back(size.value().getSExtValue());
+  op.getOperation()->setAttr(attrName,
+                             Builder(op.getContext()).getI32VectorAttr(sizes));
+}
+
+//===----------------------------------------------------------------------===//
+// Conversion patterns
+//===----------------------------------------------------------------------===//
+
+namespace {
+template <typename OpTy>
+class ExpandIfCondition : public OpRewritePattern<OpTy> {
+  using OpRewritePattern<OpTy>::OpRewritePattern;
+
+  LogicalResult matchAndRewrite(OpTy op,
+                                PatternRewriter &builder) const override {
+    // Early exit if there is no condition.
+    if (!op.ifCond())
+      return success();
+
+    // Condition is not a constant.
+    if (!op.ifCond().template getDefiningOp<ConstantOp>()) {
+      auto ifOp = builder.create<scf::IfOp>(op.getLoc(), TypeRange(),
+                                            op.ifCond(), false);
+      removeIfCondition(op);
+      ifOp.getThenBodyBuilder().clone(*op.getOperation());
+      builder.eraseOp(op);
+      return success();
+    }
+
+    // Condition is constant. Remove op if always false. Remove condition if
+    // always right.
+    auto constOp = op.ifCond().template getDefiningOp<ConstantOp>();
+    if (constOp.getValue().template cast<IntegerAttr>().getInt()) {
+      removeIfCondition(op);
+      builder.replaceOpWithNewOp<OpTy>(op, TypeRange(), op.getOperands(),
+                                       op.getOperation()->getAttrs());
+    } else {
+      builder.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());
+}
+
+namespace {
+struct ConvertOpenACCToSCFPass
+    : public ConvertOpenACCToSCFBase<ConvertOpenACCToSCFPass> {
+  void runOnOperation() override;
+};
+} // namespace
+
+void ConvertOpenACCToSCFPass::runOnOperation() {
+  auto op = getOperation();
+  auto *context = op.getContext();
+
+  RewritePatternSet patterns(context);
+  ConversionTarget target(*context);
+  populateOpenACCToSCFConversionPatterns(patterns);
+
+  target.addLegalDialect<scf::SCFDialect>();
+  target.addLegalDialect<acc::OpenACCDialect>();
+
+  target.addDynamicallyLegalOp<acc::EnterDataOp>(
+      [](acc::EnterDataOp op) { return !op.ifCond(); });
+
+  target.addDynamicallyLegalOp<acc::ExitDataOp>(
+      [](acc::ExitDataOp op) { return !op.ifCond(); });
+
+  target.addDynamicallyLegalOp<acc::UpdateOp>(
+      [](acc::UpdateOp op) { return !op.ifCond(); });
+
+  if (failed(applyPartialConversion(op, target, std::move(patterns))))
+    signalPassFailure();
+}
+
+std::unique_ptr<OperationPass<ModuleOp>> mlir::createConvertOpenACCToSCFPass() {
+  return std::make_unique<ConvertOpenACCToSCFPass>();
+}

mlir/lib/Conversion/PassDetail.h

 namespace mlir {
 class AffineDialect;
 class StandardOpsDialect;
 
 // Forward declaration from Dialect.h
 template <typename ConcreteDialect>
 void registerDialect(DialectRegistry &registry);
 
+namespace acc {
+class OpenACCDialect;
+} // end namespace acc
+
 namespace complex {
 class ComplexDialect;
 } // end namespace complex
 
 namespace gpu {
 class GPUDialect;
 class GPUModuleOp;
 } // end namespace gpu

mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp

 //===- OpenACC.cpp - OpenACC MLIR Operations ------------------------------===//
 //
 // Part of the MLIR 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/Dialect/OpenACC/OpenACC.h"
 #include "mlir/Dialect/OpenACC/OpenACCOpsEnums.cpp.inc"
+#include "mlir/Dialect/SCF/SCF.h"
 #include "mlir/IR/Builders.h"
 #include "mlir/IR/BuiltinTypes.h"
 #include "mlir/IR/OpImplementation.h"
+#include "mlir/Transforms/DialectConversion.h"
 
 using namespace mlir;
 using namespace acc;
 
 //===----------------------------------------------------------------------===//
 // OpenACC operations
 //===----------------------------------------------------------------------===//
 

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

+// RUN: mlir-opt %s -convert-openacc-to-scf -split-input-file | FileCheck %s
+
+func @testenterdataop(%a: memref<10xf32>) -> () {
+  %ifCond = constant true
+  acc.enter_data if(%ifCond) create(%a: memref<10xf32>)
+  return
+}
+
+// CHECK: acc.enter_data create(%{{.*}} : memref<10xf32>)
+
+// -----
+
+func @testenterdataop(%a: memref<10xf32>) -> () {
+  %ifCond = constant false
+  acc.enter_data if(%ifCond) create(%a: memref<10xf32>)
+  return
+}
+
+// CHECK: func @testenterdataop
+// CHECK-NOT: acc.enter_data
+
+// -----
+
+func @testenterdataop(%a: memref<10xf32>, %ifCond: i1) -> () {
+  acc.enter_data if(%ifCond) create(%a: memref<10xf32>)
+  return
+}
+
+// CHECK:      func @testenterdataop(%{{.*}}: memref<10xf32>, [[IFCOND:%.*]]: i1)
+// CHECK:        scf.if [[IFCOND]] {
+// CHECK-NEXT:     acc.enter_data create(%{{.*}} : memref<10xf32>)
+// CHECK-NEXT:   }
+
+// -----
+
+func @testexitdataop(%a: memref<10xf32>) -> () {
+  %ifCond = constant true
+  acc.exit_data if(%ifCond) delete(%a: memref<10xf32>)
+  return
+}
+
+// CHECK: acc.exit_data delete(%{{.*}} : memref<10xf32>)
+
+// -----
+
+func @testexitdataop(%a: memref<10xf32>) -> () {
+  %ifCond = constant false
+  acc.exit_data if(%ifCond) delete(%a: memref<10xf32>)
+  return
+}
+
+// CHECK: func @testexitdataop
+// CHECK-NOT: acc.exit_data
+
+// -----
+
+func @testexitdataop(%a: memref<10xf32>, %ifCond: i1) -> () {
+  acc.exit_data if(%ifCond) delete(%a: memref<10xf32>)
+  return
+}
+
+// CHECK:      func @testexitdataop(%{{.*}}: memref<10xf32>, [[IFCOND:%.*]]: i1)
+// CHECK:        scf.if [[IFCOND]] {
+// CHECK-NEXT:     acc.exit_data delete(%{{.*}} : memref<10xf32>)
+// CHECK-NEXT:   }
+
+// -----
+
+func @testupdateop(%a: memref<10xf32>) -> () {
+  %ifCond = constant true
+  acc.update if(%ifCond) host(%a: memref<10xf32>)
+  return
+}
+
+// CHECK: acc.update host(%{{.*}} : memref<10xf32>)
+
+// -----
+
+func @testupdateop(%a: memref<10xf32>) -> () {
+  %ifCond = constant false
+  acc.update if(%ifCond) host(%a: memref<10xf32>)
+  return
+}
+
+// CHECK: func @testupdateop
+// CHECK-NOT: acc.update
+
+// -----
+
+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:   }