This is an archive of the discontinued LLVM Phabricator instance.

[flang] Fix bogus message on internal subprogram with alternate return
ClosedPublic

Authored by PeteSteinfeld on Jan 4 2021, 9:46 AM.

Details

Summary

Internal subprograms have explicit interfaces. If an internal subprogram has
an alternate return, we check its explicit interface. But our explicit
interface checking was not allowing for alternate returns.

I fixed this by checking to see if the dummy argument was an alternate return.
I also added the test altreturn06.f90.

Diff Detail

Event Timeline

PeteSteinfeld requested review of this revision.Jan 4 2021, 9:46 AM
PeteSteinfeld created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 4 2021, 9:46 AM
PeteSteinfeld added a project: Restricted Project.Jan 4 2021, 9:52 AM

I'm confused by this change. The call checking code that's been modified pertains to absent actual arguments; but alternate returns can't be optional and must have corresponding positional actual arguments.

I'm confused by this change. The call checking code that's been modified pertains to absent actual arguments; but alternate returns can't be optional and must have corresponding positional actual arguments.

The code is checking for how to handle the situation where the actual argument is missing. That's potentially the case for optional arguments, but it also happens for alternate returns. So new code is saying that it's OK for the actual argument to be missing if the dummy argument is an alternate return. Formerly, we emitted an error message when calling an internal subprogram with alternate returns.

I'm confused by this change. The call checking code that's been modified pertains to absent actual arguments; but alternate returns can't be optional and must have corresponding positional actual arguments.

The code is checking for how to handle the situation where the actual argument is missing. That's potentially the case for optional arguments, but it also happens for alternate returns. So new code is saying that it's OK for the actual argument to be missing if the dummy argument is an alternate return. Formerly, we emitted an error message when calling an internal subprogram with alternate returns.

But how can it be okay for the actual argument to be missing when the corresponding dummy argument is an alternate return?

But how can it be okay for the actual argument to be missing when the corresponding dummy argument is an alternate return?

I don't know. I'll investigate further ...

With Peter's guidance, I changed the definition of actual arguments to allow
for the value of a statement label and changed the code that processes actual
arguments to put the value of the label into the actual argument.

I also verified that we were already doing the semantic checking required for
alternate returns and added another test.

klausler accepted this revision.Jan 6 2021, 12:46 PM
klausler added inline comments.
flang/lib/Semantics/expression.cpp
2875

This variable, and the code later that it enables, may no longer be needed; I'm not sure why it's there.

This revision is now accepted and ready to land.Jan 6 2021, 12:46 PM
PeteSteinfeld added inline comments.Jan 6 2021, 1:11 PM
flang/lib/Semantics/expression.cpp
2875

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.

jeanPerier requested changes to this revision.Jan 7 2021, 4:42 AM
jeanPerier added a subscriber: jeanPerier.

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

flang/include/flang/Evaluate/call.h
20

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.

flang/lib/Semantics/expression.cpp
2157

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)};

2875

I also think isAltReturn is made useless by this change and could be removed.

This revision now requires changes to proceed.Jan 7 2021, 4:42 AM

Responding to Jean's comments.

schweitz added inline comments.Jan 7 2021, 3:45 PM
flang/lib/Semantics/expression.cpp
2157

Let me know if you'd like Val to look at this one, Jean.

PeteSteinfeld added inline comments.Jan 7 2021, 5:20 PM
flang/include/flang/Evaluate/call.h
20

Thanks, Jean. I'll do that.

jeanPerier accepted this revision.Jan 8 2021, 12:36 AM

Looks good to me, thanks Pete.

This revision is now accepted and ready to land.Jan 8 2021, 12:36 AM