Page MenuHomePhabricator

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

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

Details

Summary

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.
flang/test/Semantics/acc-branch.f90
172

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.
flang/lib/Semantics/check-acc-structure.cpp
54

Why are you converting to a pointer?

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

61

Why convert to a pointer here?

79

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

181

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

182

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
flang/lib/Semantics/check-acc-structure.cpp
82

"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
flang/lib/Semantics/check-acc-structure.cpp
60

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

sameeranjoshi added inline comments.Sep 21 2020, 8:09 AM
flang/lib/Semantics/check-acc-structure.cpp
60

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

clementval added inline comments.Sep 21 2020, 5:58 PM
flang/lib/Semantics/check-acc-structure.cpp
60

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

flang/lib/Semantics/check-acc-structure.cpp
60

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.Mon, Sep 28, 9:27 AM
tskeith added inline comments.Mon, Sep 28, 10:57 AM
flang/include/flang/Semantics/tools.h
546

Is this used?

549

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.

flang/lib/Semantics/check-acc-structure.cpp
85

Function names should start with a capital letter.

93

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

103

return stmtName.source == constructName.source;

This is probably better done inline.

DavidTruby resigned from this revision.Wed, Sep 30, 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.Mon, Oct 5, 9:19 AM
flang/lib/Semantics/check-acc-structure.cpp
92

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

99

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

114

Is semantics:: necessary here?

flang/lib/Semantics/check-do-forall.cpp
859

semantics:: ?

917–918

semantics:: ?

flang/lib/Semantics/tools.cpp
1375

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?

flang/test/Semantics/acc-branch.f90
35

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.

flang/lib/Semantics/tools.cpp
1375

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.Fri, Oct 9, 3:14 AM
sameeranjoshi added inline comments.
flang/test/Semantics/acc-branch.f90
35

Thanks for pointing, it was missed out.

tskeith added inline comments.Fri, Oct 9, 9:19 AM
flang/lib/Semantics/check-do-forall.cpp
922

Can't constructName be nullopt?

flang/lib/Semantics/tools.cpp
1375

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(
    common::visitors{
        [&](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);
        },
    },
    construct);
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.Mon, Oct 12, 10:22 AM
sameeranjoshi added inline comments.
flang/lib/Semantics/check-do-forall.cpp
922

I don't think we need to handle it here, because I see it's handled inside StmtMatchesConstruct at
https://github.com/llvm/llvm-project/blob/551caec4a8af79483823e2940d40afb4c1df5da1/flang/lib/Semantics/check-do-forall.cpp#L918

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.

flang/lib/Semantics/tools.cpp
1375

Thank a lot!

tskeith added inline comments.Thu, Oct 15, 7:32 AM
flang/lib/Semantics/check-do-forall.cpp
922

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.Mon, Oct 19, 7:29 AM
tskeith added inline comments.
flang/lib/Semantics/check-do-forall.cpp
894

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.Mon, Oct 19, 9:17 AM
This revision was automatically updated to reflect the committed changes.