[acc][flang] Add recipes to data entry operations

[razvanlupusoru]

Currently recipes are held as part of the constructs. But the recipes are needed specifically for materializing semantics of the data clauses. There is no reason to not have this on the data clauses themselves.

This PR moves the recipes to the data clause entry operations themselves (acc.private, acc.firstprivate, acc.reduction).
Additionally, it removes all of the recipe entries from the data constructs themselves.

[llvmbot]

@llvm/pr-subscribers-openacc
@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-flang-fir-hlfir

Author: Razvan Lupusoru (razvanlupusoru)

Changes

Currently recipes are held as part of the constructs. But the recipes are needed specifically for materializing semantics of the data clauses. There is no reason to not have this on the data clauses themselves.

TODO: Fix the IR in flang tests which do not expect new operand
TODO: Deprecate the recipe list held on the constructs

---

Full diff: https://github.com/llvm/llvm-project/pull/149210.diff

4 Files Affected:

• (modified) flang/lib/Lower/OpenACC.cpp (+8)
• (modified) mlir/include/mlir/Dialect/OpenACC/OpenACC.h (+3)
• (modified) mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td (+21-3)
• (modified) mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp (+34)

diff --git a/flang/lib/Lower/OpenACC.cpp b/flang/lib/Lower/OpenACC.cpp
index 39e4444cde4e3..c2a884d8e4c3a 100644
--- a/flang/lib/Lower/OpenACC.cpp
+++ b/flang/lib/Lower/OpenACC.cpp
@@ -1340,6 +1340,8 @@ static void genPrivatizationRecipes(
           builder, operandLocation, info.addr, asFortran, bounds, true,
           /*implicit=*/false, mlir::acc::DataClause::acc_private, retTy, async,
           asyncDeviceTypes, asyncOnlyDeviceTypes, /*unwrapBoxAddr=*/true);
+      op.setRecipeAttr(mlir::SymbolRefAttr::get(builder.getContext(),
+                                                recipe.getSymName().str()));
       dataOperands.push_back(op.getAccVar());
     } else {
       std::string suffix =
@@ -1353,6 +1355,8 @@ static void genPrivatizationRecipes(
           /*implicit=*/false, mlir::acc::DataClause::acc_firstprivate, retTy,
           async, asyncDeviceTypes, asyncOnlyDeviceTypes,
           /*unwrapBoxAddr=*/true);
+      op.setRecipeAttr(mlir::SymbolRefAttr::get(builder.getContext(),
+                                                recipe.getSymName().str()));
       dataOperands.push_back(op.getAccVar());
     }
     privatizationRecipes.push_back(mlir::SymbolRefAttr::get(
@@ -1787,6 +1791,8 @@ genReductions(const Fortran::parser::AccObjectListWithReduction &objectList,
     mlir::acc::ReductionRecipeOp recipe =
         Fortran::lower::createOrGetReductionRecipe(
             builder, recipeName, operandLocation, ty, mlirOp, bounds);
+    op.setRecipeAttr(mlir::SymbolRefAttr::get(builder.getContext(),
+                                              recipe.getSymName().str()));
     reductionRecipes.push_back(mlir::SymbolRefAttr::get(
         builder.getContext(), recipe.getSymName().str()));
     reductionOperands.push_back(op.getAccVar());
@@ -2038,6 +2044,8 @@ privatizeIv(Fortran::lower::AbstractConverter &converter,
         builder, loc, ivValue, asFortran, {}, true, /*implicit=*/true,
         mlir::acc::DataClause::acc_private, ivValue.getType(),
         /*async=*/{}, /*asyncDeviceTypes=*/{}, /*asyncOnlyDeviceTypes=*/{});
+    op.setRecipeAttr(mlir::SymbolRefAttr::get(builder.getContext(),
+                                              recipe.getSymName().str()));
     privateOp = op.getOperation();
 
     privateOperands.push_back(op.getAccVar());
diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACC.h b/mlir/include/mlir/Dialect/OpenACC/OpenACC.h
index 4eb666239d4e4..b3a29d10e10e4 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACC.h
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACC.h
@@ -151,6 +151,9 @@ mlir::ValueRange getDataOperands(mlir::Operation *accOp);
 /// Used to get a mutable range iterating over the data operands.
 mlir::MutableOperandRange getMutableDataOperands(mlir::Operation *accOp);
 
+/// Used to get the recipe attribute from a data clause operation.
+mlir::SymbolRefAttr getRecipe(mlir::Operation *accOp);
+
 /// Used to obtain the enclosing compute construct operation that contains
 /// the provided `region`. Returns nullptr if no compute construct operation
 /// is found. The returns operation is one of types defined by
diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
index 66378f116784e..dd395912f9e7b 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
@@ -514,7 +514,8 @@ class OpenACC_DataEntryOp<string mnemonic, string clause, string extraDescriptio
           DefaultValuedAttr<BoolAttr, "false">:$implicit,
           DefaultValuedAttr<OpenACC_DataClauseModifierAttr,
             "mlir::acc::DataClauseModifier::none">:$modifiers,
-          OptionalAttr<StrAttr>:$name));
+          OptionalAttr<StrAttr>:$name,
+          OptionalAttr<SymbolRefAttr>:$recipe));
 
   let description = !strconcat(extraDescription, [{
     Description of arguments:
@@ -623,7 +624,7 @@ class OpenACC_DataEntryOp<string mnemonic, string clause, string extraDescriptio
           /*asyncOnly=*/nullptr, /*dataClause=*/nullptr,
           /*structured=*/$_builder.getBoolAttr(structured),
           /*implicit=*/$_builder.getBoolAttr(implicit), /*modifiers=*/nullptr,
-          /*name=*/nullptr);
+          /*name=*/nullptr, /*recipe=*/nullptr);
       }]>,
     OpBuilder<(ins "::mlir::Value":$var,
                    "bool":$structured, "bool":$implicit,
@@ -641,7 +642,7 @@ class OpenACC_DataEntryOp<string mnemonic, string clause, string extraDescriptio
           /*asyncOnly=*/nullptr, /*dataClause=*/nullptr,
           /*structured=*/$_builder.getBoolAttr(structured),
           /*implicit=*/$_builder.getBoolAttr(implicit), /*modifiers=*/nullptr,
-          /*name=*/$_builder.getStringAttr(name));
+          /*name=*/$_builder.getStringAttr(name), /*recipe=*/nullptr);
       }]>,
     OpBuilder<(ins "::mlir::Type":$accVarType, "::mlir::Value":$var,
                    "::mlir::Type":$varType, "::mlir::Value":$varPtrPtr,
@@ -652,10 +653,27 @@ class OpenACC_DataEntryOp<string mnemonic, string clause, string extraDescriptio
                    "::mlir::acc::DataClause":$dataClause, "bool":$structured,
                    "bool":$implicit, "::mlir::StringAttr":$name),
       [{
+        // Builder provided to ease transition for new data clause modifiers operand.
         build($_builder, $_state, accVarType, var, varType, varPtrPtr, bounds,
           asyncOperands, asyncOperandsDeviceType, asyncOnly, dataClause,
           structured, implicit, ::mlir::acc::DataClauseModifier::none, name);
       }]>,
+    OpBuilder<(ins "::mlir::Type":$accVarType, "::mlir::Value":$var,
+                   "::mlir::Type":$varType, "::mlir::Value":$varPtrPtr,
+                   "::mlir::ValueRange":$bounds,
+                   "::mlir::ValueRange":$asyncOperands,
+                   "::mlir::ArrayAttr":$asyncOperandsDeviceType,
+                   "::mlir::ArrayAttr":$asyncOnly,
+                   "::mlir::acc::DataClause":$dataClause, "bool":$structured,
+                   "bool":$implicit,
+                   "::mlir::acc::DataClauseModifier":$modifiers,
+                   "::mlir::StringAttr":$name),
+      [{
+        // Builder provided to simplify building after recipe operand was added.
+        build($_builder, $_state, accVarType, var, varType, varPtrPtr, bounds,
+              asyncOperands, asyncOperandsDeviceType, asyncOnly, dataClause,
+              structured, implicit, modifiers, name, /*recipe=*/nullptr);
+      }]>,
     ];
 }
 
diff --git a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
index f2eab62b286af..eb56d2d8933a0 100644
--- a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
+++ b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
@@ -332,6 +332,13 @@ checkValidModifier(Op op, acc::DataClauseModifier validModifiers) {
   return success();
 }
 
+template <typename Op>
+static LogicalResult checkNoRecipe(Op op) {
+  if (op.getRecipe().has_value())
+    return op.emitError("no recipes are allowed");
+  return success();
+}
+
 static ParseResult parseVar(mlir::OpAsmParser &parser,
                             OpAsmParser::UnresolvedOperand &var) {
   // Either `var` or `varPtr` keyword is required.
@@ -1063,6 +1070,24 @@ checkSymOperandList(Operation *op, std::optional<mlir::ArrayAttr> attributes,
       return op->emitOpError()
              << "expected as many " << symbolName << " symbol reference as "
              << operandName << " operands";
+    if (attributes) {
+      for (auto operandAndAttribute : llvm::zip(operands, *attributes)) {
+        mlir::Value operand = std::get<0>(operandAndAttribute);
+        if (auto *definingOp = operand.getDefiningOp()) {
+          mlir::SymbolRefAttr operandRecipe = getRecipe(definingOp);
+          // If the operand operation has a recipe - check that it is consistent
+          // with the one recorded in the construct.
+          if (operandRecipe) {
+            if (operandRecipe.getLeafReference().compare(
+                    llvm::cast<mlir::SymbolRefAttr>(
+                        std::get<1>(operandAndAttribute))
+                        .getLeafReference()) != 0)
+              return op->emitOpError() << "expected consistent recipe for "
+                                       << operandName << " operand";
+          }
+        }
+      }
+    }
   } else {
     if (attributes)
       return op->emitOpError()
@@ -4075,6 +4100,15 @@ mlir::acc::getMutableDataOperands(mlir::Operation *accOp) {
   return dataOperands;
 }
 
+mlir::SymbolRefAttr mlir::acc::getRecipe(mlir::Operation *accOp) {
+  auto recipe{
+      llvm::TypeSwitch<mlir::Operation *, mlir::SymbolRefAttr>(accOp)
+          .Case<ACC_DATA_ENTRY_OPS>(
+              [&](auto entry) { return entry.getRecipeAttr(); })
+          .Default([&](mlir::Operation *) { return mlir::SymbolRefAttr{}; })};
+  return recipe;
+}
+
 mlir::Operation *mlir::acc::getEnclosingComputeOp(mlir::Region &region) {
   mlir::Operation *parentOp = region.getParentOp();
   while (parentOp) {

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[erichkeane]

I didn't quite get all the patch, but I was wondering if this is going to reject the old mechanism? It would be great for me if it did, as that'll make sure I get it right in the clang implementation after rebase :)