[llvm-branch-commits] [clang] [flang] [flang][OpenMP] Map simple `do concurrent` loops to OpenMP host constructs (PR #127633)

[Sergio Afonso via llvm-branch-commits]

================
@@ -24,7 +25,82 @@ namespace flangomp {
 
 namespace {
 namespace looputils {
-using LoopNest = llvm::SetVector<fir::DoLoopOp>;
+/// Stores info needed about the induction/iteration variable for each `do
+/// concurrent` in a loop nest. This includes only for now:
+/// * the operation allocating memory for iteration variable,
+struct InductionVariableInfo {
+  mlir::Operation *iterVarMemDef;
+};
+
+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" opeerand 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) {
----------------
skatrak wrote:

I'm not too sure about this function. What it actually checks is that either the induction variable of `doLoop` is one of the operands of `op`, that the operation defining specifically the first operand of `op` has the induction variable as one of its operands, that the operation defining its own first operand has it, etc.

Rather than checking whether the induction variable is ultimately one of its operands, it seems to check that one of its operands is or has been obtained from (among potentially other values) the induction variable, with the seemingly arbitrary condition that, if it's obtained indirectly, it must come from a chain of "first-operands".

If I understand it correctly, the first example would meet that condition, whereas the second wouldn't. I think that these should both be treated the same:
```mlir
fir.do_loop %ind_var = ... unordered {
  %ind_var_conv = fir.convert %ind_var : (index) -> i32
  // isIndVarUltimateOperand() returns true
  %add = arith.addi %ind_var_conv, %x : i32
  ...
}
fir.do_loop %ind_var = ... unordered {
  %ind_var_conv = fir.convert %ind_var : (index) -> i32
  // isIndVarUltimateOperand() returns false
  %add = arith.addi %x, %ind_var_conv : i32
  ...
}
```
If this is intended to handle `fir.convert` and `fir.store`, I think it makes more sense to actually check for these specific operation types and usage patterns rather than creating a pseudo-generic check that can easily break. I don't think the solution is to check all operands, because at that point we're just checking whether the induction variable participated in the expression that was passed in, which doesn't seem to be what we want here.

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