[flang-commits] [PATCH] D139009: [flang] lower F77 calls in HLFIR

[Jean Perier via Phabricator via flang-commits]

jeanPerier added a comment.

Thanks for the review @PeteSteinfeld



================
Comment at: flang/include/flang/Optimizer/Builder/HLFIRTools.h:185
+/// If \p entity is a POINTER or ALLOCATABLE, dereference it and return the
+/// target entity. Return  \p entity otherwise.
+hlfir::Entity derefPointersAndAllocatables(mlir::Location loc,
----------------
PeteSteinfeld wrote:
> There's an extra space character after `Return`.
Thanks !


================
Comment at: flang/lib/Lower/Bridge.cpp:1112-1116
+      // Call statement lowering shares code with function call lowering.
+      res = Fortran::lower::createSubroutineCall(
+          *this, *stmt.typedCall, explicitIterSpace, implicitIterSpace,
+          localSymbols, stmtCtx, /*isUserDefAssignment=*/false);
+    }
----------------
PeteSteinfeld wrote:
> I don't understand this code.  This `else` corresponds to the `if` that tests to see if we're generating HLFIR.  But since the code is calling `createSubroutineCall`, I'm concerned that we're not handling the case for a function call.  Does `createSubroutineCall` handle both functions and subroutines?
There cannot be function calls here, this code (in `genFIR(const Fortran::parser::CallStmt &stmt)`) deals with fortran call statement lowering. Only subroutines can be called in a call statement.

Function calls lowering happen when lowering an Expr<T> that is a FunctionRef<T>.

The `else` case basically keeps the current lowering code during the transition. 


================
Comment at: flang/lib/Lower/ConvertCall.cpp:406-415
+/// Is this a call to an elemental procedure with at least one array argument?
+static bool
+isElementalProcWithArrayArgs(const Fortran::evaluate::ProcedureRef &procRef) {
+  if (procRef.IsElemental())
+    for (const std::optional<Fortran::evaluate::ActualArgument> &arg :
+         procRef.arguments())
+      if (arg && arg->Rank() != 0)
----------------
PeteSteinfeld wrote:
> I wonder if this is the right test.  We need to handle the case where we have an elemental function call that doesn't have any array arguments but which returns an array.
> We need to handle the case where we have an elemental function call that doesn't have any array arguments but which returns an array.

My understanding is that this is not possible ("C15101: The result of an elemental function shall be scalar" and "15.8.2 point 1: If there are no actual arguments or the actual arguments are all scalar, the result is scalar."), do you have a specific use case in mind ?

Note that a elemental function references can be assigned to an array, but in that case (when all arguments are scalars) the function returns a scalar that is assigned to all the left hand side elements. So there is no elemental aspect to deal with in such calls, the elemental aspects happen in the assignment.


================
Comment at: flang/lib/Lower/ConvertCall.cpp:417-425
+/// helper to detect statement functions
+static bool
+isStatementFunctionCall(const Fortran::evaluate::ProcedureRef &procRef) {
+  if (const Fortran::semantics::Symbol *symbol = procRef.proc().GetSymbol())
+    if (const auto *details =
+            symbol->detailsIf<Fortran::semantics::SubprogramDetails>())
+      return details->stmtFunction().has_value();
----------------
PeteSteinfeld wrote:
> I'm curious.  What's different about lowering of statement functions?
They are (and still will) be handled very differently: the statement function expression is inlined directly in lowering: we do not generate a function body and symbol for them. That is because it would be more hassle to outline statement functions than it is to inline them on the spot: statement functions do not have a body in the parse tree that would allow using the bridge code to generate a FIR function for them, it would need ad-hoc handling, whereas we already know how to lower an expression given what the referred symbols mapped to.
It is also probably better from a performance point and view. The last point is that it is possible because statement functions cannot directly or indirectly be recursive (otherwise, we would have an infinite loop in lowering).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139009/new/

https://reviews.llvm.org/D139009