[flang] Move TODO for elemental call with parentheses on argument

[peixin]

Summary

The previous TODO for elemental call with parentheses on argument is not
in the right place. Add the scenario check and move it to the right
place.

[peixin]

Following this patch, there are some more works to do.

  integer :: array(3) = (/1,2,3/), array2(3) = (/1,2,3/)
  call elemsub(array, (array(3:1:-1)), array2, (array2(3:1:-1)))
contains
  elemental subroutine elemsub(a, b, c, d)
    integer, intent(in) :: b, d
    integer, intent(out) :: a, c
    a = b + 1
    c = d + 1
  end
end

To support the above test case, I manualy change the FIR of elemental call as follows and it works correctly.

%load1 = fir.array_load %2(%4) : (!fir.ref<!fir.array<3xi32>>, !fir.shape<1>) -> !fir.array<3xi32>
%load2 = fir.array_load %3(%11) : (!fir.ref<!fir.array<3xi32>>, !fir.shape<1>) -> !fir.array<3xi32>
%res:2 = fir.do_loop %arg0 = %c0 to %14 step %c1 iter_args(%inner1 = %load1, %inner2 = %load2) -> (!fir.array<3xi32>, !fir.array<3xi32>) {
  %addr1:2 = fir.array_modify %inner1, %arg0 : (!fir.array<3xi32>, index) -> (!fir.ref<i32>, !fir.array<3xi32>)
  %37 = fir.array_fetch %10, %arg0 : (!fir.array<3xi32>, index) -> i32
  %38 = fir.no_reassoc %37 : i32
  fir.store %38 to %1 : !fir.ref<i32>
  %addr2:2 = fir.array_modify %inner2, %arg0 : (!fir.array<3xi32>, index) -> (!fir.ref<i32>, !fir.array<3xi32>)
  %41 = fir.array_fetch %17, %arg0 : (!fir.array<3xi32>, index) -> i32
  %42 = fir.no_reassoc %41 : i32
  fir.store %42 to %0 : !fir.ref<i32>
  fir.call @_QFPelemsub(%addr1#0, %1, %addr2#0, %0) : (!fir.ref<i32>, !fir.ref<i32>, !fir.ref<i32>, !fir.ref<i32>) -> ()
  fir.result %addr1#1, %addr2#1 : !fir.array<3xi32>, !fir.array<3xi32>
}
fir.array_merge_store %load1, %res#0 to %2 : !fir.array<3xi32>, !fir.array<3xi32>, !fir.ref<!fir.array<3xi32>>
fir.array_merge_store %load2, %res#1 to %3 : !fir.array<3xi32>, !fir.array<3xi32>, !fir.ref<!fir.array<3xi32>>

To get this, it seems that some more data types are missing currently. One std::function<llvm::SmallVector<fir::ExtendedValue>(const IterationSpace &)>; is needed here. The IterationSpace needs inArgs and outRess with llvm::SmallVector<mlir::Value> type. This FIXME needs to be fixed.

Is the above analysis the right direction? Is there someone working on this?

2. For the following test case, gfortran report warnings.

  integer :: array(3) = (/1,2,3/), array2(3) = (/1,2,3/)
  call elemsub(array, array(3:1:-1), array2, array2(3:1:-1))
  print *, array
  print *, array2
contains
  elemental subroutine elemsub(a, b, c, d)
    integer, intent(in) :: b, d
    integer, intent(out) :: a, c
    a = b + 1
    c = d + 1
  end
end

Should we also report warnings for the elemental call with conflict storage? If yes, can I work on this?

[jeanPerier]

LGTM, Thanks.

Is the above analysis the right direction? Is there someone working on this?

I think it is going the right direction, yes. Please wait, @schweitz confirmation here, but I do not think anyone is working on this currently.

Should we also report warnings for the elemental call with conflict storage? If yes, can I work on this?

I do not see this as a necessity since the compiler will not be able to catch all conflicts (e.g, if pointers are involved, or if the slice indices are not compiled time constants). So I would not prioritize it, but it looks like a nice to have.

[peixin]

In D125632#3514991, @jeanPerier wrote:

LGTM, Thanks.

Is the above analysis the right direction? Is there someone working on this?

I think it is going the right direction, yes. Please wait, @schweitz confirmation here, but I do not think anyone is working on this currently.

OK.

Should we also report warnings for the elemental call with conflict storage? If yes, can I work on this?

I do not see this as a necessity since the compiler will not be able to catch all conflicts (e.g, if pointers are involved, or if the slice indices are not compiled time constants). So I would not prioritize it, but it looks like a nice to have.

I did see a lot of code using unspecified or semantically wrong behaviors in real workloads when we migrated them from gfortran/ifort to classic flang. The semantic warnings and debug helpers such as backtrace extension is really helpful for finding one bug in a large workloads. So personally, it is better for me to have more semantic checks in the long term. Considering that I have written assignmentHasMemConflict in D124737, it should not take much more work to support this by expanding it. I will give it a try.

I plan to look at equivalence, pointer, associate construct, common block members by use association with the same offset. For array sections with non-compiled time constants indices, it should need runtime check, which may or may not be supported in future.

[schweitz]

array_modify is deprecated. It is no longer needed.

This change looks like the right direction to me. Thank you for the fix.

Ultimately, yes, the AEL framework will need to correctly thread more than a single output array through the compute loop nest. (There are a few artifacts sitting around that point in that direction.) I don't think we want to change the continuations such that they will generate a series of array values, but rather manage the distinct array values respective to the context. In your example, the elemental subroutine, elemsub, needs to be correctly analyzed as having two outputs, each one being an array that must be threaded via the result backedge. That should translate into AEL having a vector of elemental values (more than 1 rhs value) that the elemental procedure call needs to thread to the appropriate array_amend and result, but not lowering all arrays "at once" with each expression traversal. Most (neighboring all) Fortran array expressions will evaluate a single array result or otherwise be ill-defined Fortran with multiple update violations. But there are cases such as user-defined elemental subroutines that are exceptions, of course.

[peixin]

In D125632#3520671, @schweitz wrote:

array_modify is deprecated. It is no longer needed.

This change looks like the right direction to me. Thank you for the fix.

Ultimately, yes, the AEL framework will need to correctly thread more than a single output array through the compute loop nest. (There are a few artifacts sitting around that point in that direction.) I don't think we want to change the continuations such that they will generate a series of array values, but rather manage the distinct array values respective to the context. In your example, the elemental subroutine, elemsub, needs to be correctly analyzed as having two outputs, each one being an array that must be threaded via the result backedge. That should translate into AEL having a vector of elemental values (more than 1 rhs value) that the elemental procedure call needs to thread to the appropriate array_amend and result, but not lowering all arrays "at once" with each expression traversal. Most (neighboring all) Fortran array expressions will evaluate a single array result or otherwise be ill-defined Fortran with multiple update violations. But there are cases such as user-defined elemental subroutines that are exceptions, of course.

Thanks for the detailed explanations. I need to and will take a deeper look at FIR array ops related lowering to support this feature. But please feel free to take up this if anyone has free time and wants to support this.

@schweitz Is it OK to land this patch?

[peixin]

@jeanPerier @schweitz Sorry to bother you again for this issue.

Previously, I mainly tested gfortran and ifort, and did not check classic-flang too much. I checked classic-flang for this issue. It does not support this, either. We tested a lot of workloads using classic-flang and did not get the error for this. So I take it as edge case, and plan not to work on recently.

I tested a lot of F95 test cases. The good news is LLVM Flang in fir-dev performs kind of better than gfortran and classic-flang except for some TODOs. Next, I will include the tests for derived type (including PDT), interoperability with C, and some simple F2003 features. Please let me know if there are some features you cares more and I am happy to make some checks and tests.

[schweitz]

In D125632#3543959, @peixin wrote:

@jeanPerier @schweitz Sorry to bother you again for this issue.

Previously, I mainly tested gfortran and ifort, and did not check classic-flang too much. I checked classic-flang for this issue. It does not support this, either. We tested a lot of workloads using classic-flang and did not get the error for this. So I take it as edge case, and plan not to work on recently.

I tested a lot of F95 test cases. The good news is LLVM Flang in fir-dev performs kind of better than gfortran and classic-flang except for some TODOs. Next, I will include the tests for derived type (including PDT), interoperability with C, and some simple F2003 features. Please let me know if there are some features you cares more and I am happy to make some checks and tests.

Thanks for the feedback, @peixin. It's good to get independent evidence and confirmation that AEL has good support for this modulo the TODOs, etc.

flang/lib/Lower/ConvertExpr.cpp

 }
 template <>
 bool isElementalProcWithArrayArgs(const Fortran::lower::SomeExpr &x) {
   if (const auto *procRef = std::get_if<Fortran::evaluate::ProcedureRef>(&x.u))
     return isElementalProcWithArrayArgs(*procRef);
   return false;
 }
 
+/// Is this a call to an elemental procedure with at least one argument in
+/// parentheses?
+static bool
+isElementalProcWithParenArgs(const Fortran::evaluate::ProcedureRef &procRef) {
+  if (procRef.IsElemental())
+    for (const std::optional<Fortran::evaluate::ActualArgument> &arg :
+         procRef.arguments())
+      if (arg && isParenthesizedVariable(*arg.value().UnwrapExpr()))
+        return true;
+  return false;
+}
+
 /// Some auxiliary data for processing initialization in ScalarExprLowering
 /// below. This is currently used for generating dense attributed global
 /// arrays.
 struct InitializerData {
   explicit InitializerData(bool getRawVals = false) : genRawVals{getRawVals} {}
   llvm::SmallVector<mlir::Attribute> rawVals; // initialization raw values
   mlir::Type rawType; // Type of elements processed for rawVals vector.
   bool genRawVals;    // generate the rawVals vector if set.
lowerElementalSubroutine(Fortran::lower::AbstractConverter &converter,
                            Fortran::lower::SymMap &symMap,
                            Fortran::lower::StatementContext &stmtCtx,
                            const Fortran::lower::SomeExpr &call) {
     ArrayExprLowering ael(converter, stmtCtx, symMap,
                           ConstituentSemantics::RefTransparent);
     ael.lowerElementalSubroutine(call);
   }
 
-  // TODO: See the comment in genarr(const Fortran::lower::Parentheses<T>&).
-  // This is skipping generation of copy-in/copy-out code for analysis that is
-  // required when arguments are in parentheses.
   void lowerElementalSubroutine(const Fortran::lower::SomeExpr &call) {
+    assert(isElementalProcWithArrayArgs(call) &&
+           "no array argument in elemental call");
+    if (const auto *procRef =
+            std::get_if<Fortran::evaluate::ProcedureRef>(&call.u)) {
+      if (isElementalProcWithParenArgs(*procRef)) {
+        // TODO: All array arguments with INTENT(OUT) or INTENT(INOUT) should
+        // use array_modify and array_merge_store ops.
+        TODO(getLoc(), "parentheses on argument in elemental call");
+      }
+    }
     auto f = genarr(call);
     llvm::SmallVector<mlir::Value> shape = genIterationShape();
     auto [iterSpace, insPt] = genImplicitLoops(shape, /*innerArg=*/{});
     f(iterSpace);
     finalizeElementCtx();
     builder.restoreInsertionPoint(insPt);
   }
 
return [=](IterSpace iters) -> ExtValue {
       return fir::factory::Complex{builder, loc}.extractComplexPart(lhs,
                                                                     isImagPart);
     };
   }
 
   template <typename T>
   CC genarr(const Fortran::evaluate::Parentheses<T> &x) {
     mlir::Location loc = getLoc();
-    if (isReferentiallyOpaque()) {
-      // Context is a call argument in, for example, an elemental procedure
-      // call. TODO: all array arguments should use array_load, array_access,
-      // array_amend, and INTENT(OUT), INTENT(INOUT) arguments should have
-      // array_merge_store ops.
-      TODO(loc, "parentheses on argument in elemental call");
-    }
     auto f = genarr(x.left());
     return [=](IterSpace iters) -> ExtValue {
       auto val = f(iters);
       mlir::Value base = fir::getBase(val);
       auto newBase =
           builder.create<fir::NoReassocOp>(loc, base.getType(), base);
       return fir::substBase(val, newBase);
     };