[PATCH] D94017: [flang] Fix bogus message on internal subprogram with alternate return

[Jean Perier via Phabricator via llvm-commits]

jeanPerier requested changes to this revision.
jeanPerier added a comment.
This revision now requires changes to proceed.

> @schweitz, as I understand things, the current lowering code gets the value of alternate return labels from the parse tree. Do you plan to change that once their available in the values of the ActualArgument?

That would make sens IMHO.
Otherwise, the change you have should work OK with lowering, except for the ProcedureRef hasAlternateReturns that is now wrong and would make lowering fail (see inlined comment).



================
Comment at: flang/include/flang/Evaluate/call.h:19
 #include "flang/Parser/char-block.h"
+#include "flang/Parser/parse-tree.h"
 #include "flang/Semantics/attr.h"
----------------
parse-tree.h is pretty big, have you seen any compile time increase with this include here ? If so, you could define Label in `lib/Common/Fortran.h` and use that here and in parse-tree.h to save the parse-tree.h include here.



================
Comment at: flang/lib/Semantics/expression.cpp:2148
         bool hasAlternateReturns{
             callee->arguments.size() < actualArgList.size()};
         callStmt.typedCall.Reset(
----------------
That part of the code is now wrong (since argument list now contains the labels). This will yield wrong lowering. You could replace it with a call to a helper like:

```
static bool HasAlternateReturns(const evaluate::ActualArguments &args) {
  for (const auto &arg : args) {
    if (arg && arg->isAlternateReturn()) {
      return true;
    }
  }
  return false;
}
```

and `bool hasAlternateReturns{HasAlternateReturns(callee->arguments)};`


================
Comment at: flang/lib/Semantics/expression.cpp:2868
+                   actual = ActualArgument(x.v);
                    isAltReturn = true;
                  },
----------------
PeteSteinfeld wrote:
> klausler wrote:
> > This variable, and the code later that it enables, may no longer be needed; I'm not sure why it's there.
> It does seem wrong to create data that's never used.
> 
> @schweitz, as I understand things, the current lowering code gets the value of alternate return labels from the parse tree.  Do you plan to change that once their available in the values of the ActualArgument?
> 
> Note that there are a couple of ways that this value is used.  One is for the implementation of the function isAlternateReturn().  The other is that you can now dump out the value of the label in the actual argument if you dump it out.
> 
> 
I also think `isAltReturn` is made useless by this change  and could be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94017