Page MenuHomePhabricator

[Flang][OpenACC] Fix for branching out issue in OpenACC parallel construct.

Authored by sameeranjoshi on Sep 18 2020, 7:46 AM.



From OpenACC 3.0 Standards document
840 • A program may not branch into or out of an OpenACC parallel construct.
Exits are allowed provided it does not cause an exit outside the parallel region.

Test case exits out of the inner do loop, but it is still inside the parallel region.
Patch tries to extract labels from block attached to a construct,
If the exit is to a label not in the collected list then flags an error.

Diff Detail

Event Timeline

sameeranjoshi created this revision.Sep 18 2020, 7:46 AM
Herald added a project: Restricted Project. · View Herald Transcript
sameeranjoshi requested review of this revision.Sep 18 2020, 7:46 AM
clementval requested changes to this revision.Sep 18 2020, 7:53 AM
clementval added inline comments.

Can you add a test where the EXIT is out of the parallel construct probably with a label.

This revision now requires changes to proceed.Sep 18 2020, 7:53 AM
tskeith requested changes to this revision.Sep 18 2020, 9:11 AM
tskeith added a subscriber: tskeith.
tskeith added inline comments.

Why are you converting to a pointer?

I suggest:
if (const auto &name{std::get<std::optional<parser::Name>>(doStmt.statement.t)}) {


Why convert to a pointer here?


This message needs to be fixed to match the change. RETURN and EXIT are now allowed in some cases so it is not accurate.


doConstructNameSet would be a much better name. Then you don't need comments explaining why you chose a confusing name.


Can't you do this as part of NoBranchingEnforce? There is a lot of overhead to having another parse tree visitor.

tskeith added inline comments.Sep 18 2020, 9:25 AM

"Enclosing block construct" doesn't make sense here. It's not in a block construct.

Pleas add test like the following one where we check the error:

program test
  integer :: i, j
  label1: do i = 1, 10
    !$acc parallel loop
    do j = 1, 10
      if (j == 2) then
        exit label1
      end if
    end do
  end do label1
end program test
sameeranjoshi marked 7 inline comments as done.

Worked on review comments.

clementval added inline comments.Sep 21 2020, 7:52 AM

I guess they are others construct that can have a label.

sameeranjoshi added inline comments.Sep 21 2020, 8:09 AM

Do you mean collecting labels from all the statements in the block?

clementval added inline comments.Sep 21 2020, 5:58 PM

I don't have an example right here but it's probable that an exit statement can exit to other constructs label than DoConstruct.


Can you check lib/Semantics/check-do-forall.cpp for similar functionality? Particularly, the following functions.
void DoForallChecker::Enter(const parser::ExitStmt &exitStmt) {

CheckNesting(StmtType::EXIT, common::GetPtrFromOptional(exitStmt.v));

void DoForallChecker::CheckNesting

Worked on below points:

  1. Moved some helper functions to tools.h which can be shared between OpenMP/OpenACC and Fortran constructs.
  2. Emit a more better error message.
  3. Added more tests.
  4. Used ConstructStack to check labels.
tskeith added a project: Restricted Project.Sep 28 2020, 9:27 AM
tskeith added inline comments.Sep 28 2020, 10:57 AM

Is this used?


It looks like the only place that MaybeGetConstructName is used is in MaybeGetNodeName, so how about just doing it inline there? I don't see a benefit of making it part of the API of tools.h.


Function names should start with a capital letter.


That is not a label, it's a construct name. It confuses the user and the code to call things labels that are not.


return stmtName.source == constructName.source;

This is probably better done inline.

DavidTruby resigned from this revision.Sep 30 2020, 5:35 AM
sameeranjoshi marked 7 inline comments as done.

Address review comments.

@tskeith are the changes ok?
I have the same patch extended for OpenMP(D88655).

tskeith added inline comments.Oct 5 2020, 9:19 AM

I suggest "%s to construct '%s' outside of..."


I meant do the comparison in-line, and not define this function. Hiding it in a function makes it more obscure, IMO.


Is semantics:: necessary here?


semantics:: ?


semantics:: ?


The call to common::GetPtrFromOptional could be moved outside of std::visit so that it doesn't have to be repeated in each lambda.

Even better might be to change the return type to const <std::optional<parser::Name>> &. Is there any reason to convert it to a pointer?


You're still using the word "label" incorrectly here. There are no labels in this test.

@sameeranjoshi Do you have time to address latest review comments?

Work on review comments.

@sameeranjoshi Do you have time to address latest review comments?

Sorry for holding this for a long, I was trying to solve unknown regressions with respect to check-do-forall.cpp after the current suggested set of changes.

See issue mentioned inline.


My knowledge of modern cpp seemed to pull me back.
I tried various different ways to return const std::optional<parser::Name> &, all resulting in either seg-fault or unknown regressions in fortran loops.
I have managed to do this change using a return-by-value and not return-by-reference.
If these changes still need to be modified, some suggestions on using a return-by-reference would be helpful.

sameeranjoshi marked 6 inline comments as done.Oct 9 2020, 3:14 AM
sameeranjoshi added inline comments.

Thanks for pointing, it was missed out.

tskeith added inline comments.Oct 9 2020, 9:19 AM

Can't constructName be nullopt?


You may need to provide return types for the lambdas so that std::visit returns a reference and not a value. Like this:

return std::visit(
        [&](const parser::BlockConstruct *blockConstruct)
            -> const std::optional<parser::Name> & {
          return std::get<0>(blockConstruct->t).statement.v;
        [&](const auto *a) -> const std::optional<parser::Name> & {
          return std::get<0>(std::get<0>(a->t).statement.t);
sameeranjoshi marked an inline comment as done.

Address a few more reviews.
Changed to return a reference from MaybeGetNodeName.

sameeranjoshi marked 3 inline comments as done.Oct 12 2020, 10:22 AM
sameeranjoshi added inline comments.

I don't think we need to handle it here, because I see it's handled inside StmtMatchesConstruct at

I also tried to use the conventional approach of using a guard for std::optional.

if (constructName){
// Accessing the std::optional value by dereferencing it.


But I see regressions due to CheckDoConcurrentExit, which seem to have a dependency on the result from StmtMatchesConstruct.
Please correct me if I am wrong here.


Thank a lot!

tskeith added inline comments.Oct 15 2020, 7:32 AM

It's too late to handle it in StmtMatchesConstruct. It's dereferenced before StmtMatchesConstruct is called. *constructName is undefined if constructName is nullopt.

It would simplify things if that argument were an optional rather than a pointer.

sameeranjoshi marked an inline comment as done.

Change constructName argument of StmtMatchesConstruct from pointer to std::optional to avoid undefined behaviour.

tskeith accepted this revision.Oct 19 2020, 7:29 AM
tskeith added inline comments.

The original is better. constructName is checked for null so .value() is unnecessary.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 19 2020, 9:17 AM
This revision was automatically updated to reflect the committed changes.