Handle review comments

Author: ergawy

flang/lib/Optimizer/OpenMP/DoConcurrentConversion.cpp

@@ -35,39 +35,6 @@ struct InductionVariableInfo {
 using LoopNestToIndVarMap =
     llvm::MapVector<fir::DoLoopOp, InductionVariableInfo>;
 
-/// Given an operation `op`, this returns true if one of `op`'s operands is
-/// "ultimately" the loop's induction variable. This helps in cases where the
-/// induction variable's use is "hidden" behind a convert/cast.
-///
-/// For example, give the following loop:
-/// ```
-///   fir.do_loop %ind_var = %lb to %ub step %s unordered {
-///     %ind_var_conv = fir.convert %ind_var : (index) -> i32
-///     fir.store %ind_var_conv to %i#1 : !fir.ref<i32>
-///     ...
-///   }
-/// ```
-///
-/// If \p op is the `fir.store` operation, then this function will return true
-/// since the IV is the "ultimate" operand to the `fir.store` op through the
-/// `%ind_var_conv` -> `%ind_var` conversion sequence.
-///
-/// For why this is useful, see its use in `findLoopIndVarMemDecl`.
-bool isIndVarUltimateOperand(mlir::Operation *op, fir::DoLoopOp doLoop) {
-  while (op != nullptr && op->getNumOperands() > 0) {
-    auto ivIt = llvm::find_if(op->getOperands(), [&](mlir::Value operand) {
-      return operand == doLoop.getInductionVar();
-    });
-
-    if (ivIt != op->getOperands().end())
-      return true;
-
-    op = op->getOperand(0).getDefiningOp();
-  }
-
-  return false;
-}
-
 /// For the \p doLoop parameter, find the operation that declares its iteration
 /// variable or allocates memory for it.
 ///
@@ -84,19 +51,20 @@ bool isIndVarUltimateOperand(mlir::Operation *op, fir::DoLoopOp doLoop) {
 /// ```
 ///
 /// This function returns the `hlfir.declare` op for `%i`.
+///
+/// Note: The current implementation is dependent on how flang emits loop
+/// bodies; which is sufficient for the current simple test/use cases. If this
+/// proves to be insufficient, this should be made more generic.
 mlir::Operation *findLoopIterationVarMemDecl(fir::DoLoopOp doLoop) {
   mlir::Value result = nullptr;
-  mlir::visitUsedValuesDefinedAbove(
-      doLoop.getRegion(), [&](mlir::OpOperand *operand) {
-        if (result)
-          return;
-
-        if (isIndVarUltimateOperand(operand->getOwner(), doLoop)) {
-          assert(result == nullptr &&
-                 "loop can have only one induction variable");
-          result = operand->get();
-        }
-      });
+  for (mlir::Operation &op : doLoop) {
+    // The first `fir.store` op we come across should be the op that updates the
+    // loop's iteration variable.
+    if (auto storeOp = mlir::dyn_cast<fir::StoreOp>(op)) {
+      result = storeOp.getMemref();
+      break;
+    }
+  }
 
   assert(result != nullptr && result.getDefiningOp() != nullptr);
   return result.getDefiningOp();
@@ -239,6 +207,10 @@ class DoConcurrentConversion : public mlir::OpConversionPattern<fir::DoLoopOp> {
   mlir::LogicalResult
   matchAndRewrite(fir::DoLoopOp doLoop, OpAdaptor adaptor,
                   mlir::ConversionPatternRewriter &rewriter) const override {
+    if (mapToDevice)
+      return doLoop.emitError(
+          "not yet implemented: Mapping `do concurrent` loops to device");
+
     looputils::LoopNestToIndVarMap loopNest;
     bool hasRemainingNestedLoops =
         failed(looputils::collectLoopNest(doLoop, loopNest));
@@ -407,8 +379,6 @@ class DoConcurrentConversionPass
 
     if (mlir::failed(mlir::applyFullConversion(getOperation(), target,
                                                std::move(patterns)))) {
-      mlir::emitError(mlir::UnknownLoc::get(context),
-                      "error in converting do-concurrent op");
       signalPassFailure();
     }
   }

flang/test/Transforms/DoConcurrent/basic_device.mlir

@@ -0,0 +1,29 @@
+// RUN: fir-opt --omp-do-concurrent-conversion="map-to=device" -verify-diagnostics %s
+
+func.func @do_concurrent_basic() attributes {fir.bindc_name = "do_concurrent_basic"} {
+    %0 = fir.alloca i32 {bindc_name = "i"}
+    %1:2 = hlfir.declare %0 {uniq_name = "_QFEi"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+    %2 = fir.address_of(@_QFEa) : !fir.ref<!fir.array<10xi32>>
+    %c10 = arith.constant 10 : index
+    %3 = fir.shape %c10 : (index) -> !fir.shape<1>
+    %4:2 = hlfir.declare %2(%3) {uniq_name = "_QFEa"} : (!fir.ref<!fir.array<10xi32>>, !fir.shape<1>) -> (!fir.ref<!fir.array<10xi32>>, !fir.ref<!fir.array<10xi32>>)
+    %c1_i32 = arith.constant 1 : i32
+    %7 = fir.convert %c1_i32 : (i32) -> index
+    %c10_i32 = arith.constant 10 : i32
+    %8 = fir.convert %c10_i32 : (i32) -> index
+    %c1 = arith.constant 1 : index
+
+    // expected-error@+2 {{not yet implemented: Mapping `do concurrent` loops to device}}
+    // expected-error@below {{failed to legalize operation 'fir.do_loop'}}
+    fir.do_loop %arg0 = %7 to %8 step %c1 unordered {
+      %13 = fir.convert %arg0 : (index) -> i32
+      fir.store %13 to %1#1 : !fir.ref<i32>
+      %14 = fir.load %1#0 : !fir.ref<i32>
+      %15 = fir.load %1#0 : !fir.ref<i32>
+      %16 = fir.convert %15 : (i32) -> i64
+      %17 = hlfir.designate %4#0 (%16)  : (!fir.ref<!fir.array<10xi32>>, i64) -> !fir.ref<i32>
+      hlfir.assign %14 to %17 : i32, !fir.ref<i32>
+    }
+
+    return
+  }