[flang-commits] [flang] [mlir] [OpenACC] Remove attribute verification (PR #137674)

[via flang-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-flang-fir-hlfir

@llvm/pr-subscribers-mlir

Author: Susan Tan (ス-ザン　タン) (SusanTan)

<details>
<summary>Changes</summary>

The part that verifies the declare attributes are preserved in the verifier can fail easily during a FIR lowering pipeline. For example, during FIR lowering to FIRCG, fir.declare can be removed, thus any fir.declare that has acc.declare attributes will cause a verifier failure. Since the declare attribute only existed to simplify the effort of locating acc declare enter and exit points, which can be easily replaced by a def-use chain traversal, we are thinking of removing the verification of declae attributes in this MR. Example:

`  %1 = fir.alloca !fir.array<10xf32> {bindc_name = "arr", uniq_name = "_QMmmFsubEarr"}
  %2 = fir.shape %c10 : (index) -> !fir.shape<1>
  %3 = fir.declare %1(%2) {acc.declare = #acc.declare<dataClause =  acc_create>, uniq_name = "_QMmmFsubEarr"} : (!fir.ref<!fir.array<10xf32>>, !fir.shape<1>) -> !fir.ref<!fir.array<10xf32>>
  %4 = acc.create varPtr(%3 : !fir.ref<!fir.array<10xf32>>) -> !fir.ref<!fir.array<10xf32>> {name = "arr"}
  %5 = acc.declare_enter dataOperands(%4 : !fir.ref<!fir.array<10xf32>>)`

the acc.declare_enter itself is enough to identify when the data region starts.

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


3 Files Affected:

- (modified) flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp (+6-1) 
- (modified) flang/test/Fir/declare-codegen.fir (+14-3) 
- (modified) mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp (-26) 


``````````diff
diff --git a/flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp b/flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp
index d09d7d397e8b7..587bcaf718c0b 100644
--- a/flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp
+++ b/flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp
@@ -299,11 +299,16 @@ class DeclareOpConversion : public mlir::OpRewritePattern<fir::DeclareOp> {
       else
         return mlir::failure();
     }
-    // FIXME: Add FortranAttrs and CudaAttrs
+
     auto xDeclOp = rewriter.create<fir::cg::XDeclareOp>(
         loc, declareOp.getType(), declareOp.getMemref(), shapeOpers, shiftOpers,
         declareOp.getTypeparams(), declareOp.getDummyScope(),
         declareOp.getUniqName());
+
+    // attach metadatas from fir.declare to fircg.ext_declare
+    for (const mlir::NamedAttribute &attr : declareOp->getAttrs())
+      xDeclOp->setAttr(attr.getName(), attr.getValue());
+
     LLVM_DEBUG(llvm::dbgs()
                << "rewriting " << declareOp << " to " << xDeclOp << '\n');
     rewriter.replaceOp(declareOp, xDeclOp.getOperation()->getResults());
diff --git a/flang/test/Fir/declare-codegen.fir b/flang/test/Fir/declare-codegen.fir
index fe8d84ef2d19f..cf4f28eda1cb4 100644
--- a/flang/test/Fir/declare-codegen.fir
+++ b/flang/test/Fir/declare-codegen.fir
@@ -23,7 +23,7 @@ func.func private @bar(%arg0: !fir.ref<!fir.array<12x23xi32>>)
 
 // DECL-LABEL: func.func @test(
 // DECL-SAME: %[[arg0:.*]]: !fir.ref<!fir.array<12x23xi32>>) {
-// DECL: fircg.ext_declare
+// DECL: fircg.ext_declare{{.*}}{uniq_name = "_QFarray_numeric_lboundsEx"}
 
 
 func.func @useless_shape_with_duplicate_extent_operand(%arg0: !fir.ref<!fir.array<3x3xf32>>) {
@@ -37,7 +37,7 @@ func.func @useless_shape_with_duplicate_extent_operand(%arg0: !fir.ref<!fir.arra
 // NODECL-NEXT: return
 
 // DECL-LABEL: func.func @useless_shape_with_duplicate_extent_operand(
-// DECL: fircg.ext_declare
+// DECL: fircg.ext_declare{{.*}}{uniq_name = "u"}
 
 // Test DCE does not crash because of unreachable code.
 func.func @unreachable_code(%arg0: !fir.ref<!fir.char<1,10>>) {
@@ -51,4 +51,15 @@ func.func @unreachable_code(%arg0: !fir.ref<!fir.char<1,10>>) {
 // NODECL-LABEL:   func.func @unreachable_code(
 // NODECL-NOT:       uniq_name = "live_code"
 // DECL-LABEL:    func.func @unreachable_code(
-// DECL:             uniq_name = "live_code"
+// DECL:             fircg.ext_declare{{.*}}{uniq_name = "live_code"}
+// DECL:             fir.declare{{.*}}{uniq_name = "dead_code"}
+
+
+// Test that attributes get attached to the memref operation when fir.declare is not preserved
+func.func @test_local_memref() {
+  %0 = fir.alloca !fir.array<10xi32> {uniq_name = "local_array"}
+  %1 = fir.declare %0 {uniq_name = "local_array_declare", acc.declare = #acc.declare<dataClause = acc_create>} : (!fir.ref<!fir.array<10xi32>>) -> !fir.ref<!fir.array<10xi32>>
+  return
+}
+// DECL-LABEL: func.func @test_local_memref() {
+// DECL: fircg.ext_declare{{.*}}{acc.declare = #acc.declare<dataClause = acc_create>, uniq_name = "local_array_declare"}
diff --git a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
index 56f3228d3a652..66584dc708b8c 100644
--- a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
+++ b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
@@ -2910,32 +2910,6 @@ checkDeclareOperands(Op &op, const mlir::ValueRange &operands,
     assert(dataClauseOptional.has_value() &&
            "declare operands can only be data entry operations which must have "
            "dataClause");
-
-    // If varPtr has no defining op - there is nothing to check further.
-    if (!var.getDefiningOp())
-      continue;
-
-    // Check that the varPtr has a declare attribute.
-    auto declareAttribute{
-        var.getDefiningOp()->getAttr(mlir::acc::getDeclareAttrName())};
-    if (!declareAttribute)
-      return op.emitError(
-          "expect declare attribute on variable in declare operation");
-
-    auto declAttr = mlir::cast<mlir::acc::DeclareAttr>(declareAttribute);
-    if (declAttr.getDataClause().getValue() != dataClauseOptional.value())
-      return op.emitError(
-          "expect matching declare attribute on variable in declare operation");
-
-    // If the variable is marked with implicit attribute, the matching declare
-    // data action must also be marked implicit. The reverse is not checked
-    // since implicit data action may be inserted to do actions like updating
-    // device copy, in which case the variable is not necessarily implicitly
-    // declare'd.
-    if (declAttr.getImplicit() &&
-        declAttr.getImplicit() != acc::getImplicitFlag(operand.getDefiningOp()))
-      return op.emitError(
-          "implicitness must match between declare op and flag on variable");
   }
 
   return success();

``````````

</details>


https://github.com/llvm/llvm-project/pull/137674