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

[Pete Steinfeld via Phabricator via flang-commits]

PeteSteinfeld accepted this revision.
PeteSteinfeld added a comment.
This revision is now accepted and ready to land.

Aside from nits and questions, all builds and tests correctly and looks good.



================
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,
----------------
There's an extra space character after `Return`.


================
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);
+    }
----------------
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?


================
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)
----------------
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.


================
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();
----------------
I'm curious.  What's different about lowering of statement functions?


================
Comment at: flang/lib/Optimizer/Builder/HLFIRTools.cpp:173
+  mlir::Value source = value;
+  /// Lowered scalar expression values for numerical  and logical may have a
+  /// different type than what is required for the type in memory (logical
----------------
There's an extra space character after `numerical`.


================
Comment at: flang/lib/Optimizer/Builder/HLFIRTools.cpp:177
+  /// according to the fir.logical<kind> so that the storage size is correct).
+  /// Character length mismatch are ignored (it is ok for one to be dynamic and
+  /// the other static).
----------------
"mismatch" should be "mismatches".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139009