Page MenuHomePhabricator

[flang][openacc] Enforce restriction on routine directive and clauses
ClosedPublic

Authored by clementval on Dec 4 2020, 10:34 AM.

Details

Summary

This patch add some checks for the restriction on the routine directive
and fix several issue at the same time.

Validity tests have been added in a separate file than acc-clause-validity.f90 since this one
became quite large. I plan to split the larger file once on-going review are done.

Diff Detail

Event Timeline

clementval created this revision.Dec 4 2020, 10:34 AM
clementval requested review of this revision.Dec 4 2020, 10:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 4 2020, 10:34 AM
clementval added a project: Restricted Project.Dec 4 2020, 10:34 AM

Thanks for patch.
A few comments below.

flang/include/flang/Parser/parse-tree.h
3857

Sorry I couldn't find where is this defined in standard.
Could you please point that?

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

Where is the check for inside subroutine/function as well as inside interface done?
Sorry I might be missing something, but I see you check only for the module scope of the construct.

246

part

flang/test/Semantics/acc-routine-validity.f90
20

Why isn't it getting resolved though having a resolving function defined above?
Do you think there is a need for a more better resolver, which I tried in
https://reviews.llvm.org/D93051 with the latest diff?

80

Tests for allowedOnceClauses ?

clementval marked an inline comment as done.Dec 16 2020, 6:16 AM
clementval added inline comments.
flang/include/flang/Parser/parse-tree.h
3857

3.1 line 2732 and 2733

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

The only place where you cannot have routine directive without a name is in the module specification part. So the test here is kind of inverted if you want. Instead of checking where it's allowed, I'm checking where it is not.

flang/test/Semantics/acc-routine-validity.f90
20

dummy is not defined anywhere and must raise an error here. There is no subroutine/function named dummy so no symbol so the error is triggered. This is the correct behavior. Not all name can be resolved if they really do not exists.

sameeranjoshi added inline comments.Dec 16 2020, 10:34 AM
flang/test/Semantics/acc-routine-validity.f90
20

So why is it an internal error, which seems to be a fatal error?
Shouldn't it be something like no declaration found or the normal way we get errors from Fortran part in F18?

clementval marked an inline comment as done.Dec 16 2020, 10:50 AM
clementval added inline comments.
flang/test/Semantics/acc-routine-validity.f90
20

Well this is the Flang error for no symbol found why would you want a different error? I don't get your point here.

tskeith added inline comments.Dec 16 2020, 11:11 AM
flang/test/Semantics/acc-routine-validity.f90
20

That message is an internal error, not a user error. An internal error means a bug in the compiler, not in the user's program.

This particular internal error means that we have not resolved dummy to a symbol but we also have not reported any errors to the user indicating what is wrong.

clementval added inline comments.Dec 16 2020, 11:13 AM
flang/test/Semantics/acc-routine-validity.f90
20

Alright. I'm gonna issue a proper user error for this case. Thanks for the precision @tskeith

Address review comment + rebase

clementval marked 3 inline comments as done.Dec 16 2020, 12:33 PM
clementval added inline comments.
flang/test/Semantics/acc-routine-validity.f90
20

@sameeranjoshi User error message is now issued instead of the internal error.

sameeranjoshi accepted this revision.Dec 17 2020, 2:26 AM

LGTM.
Please take care of the remaining nits comments before merging.
Thank you @tskeith for pointers.

This revision is now accepted and ready to land.Dec 17 2020, 2:26 AM
clementval marked an inline comment as done.

Rebase + nit comment

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

Good catch! Thx!

This revision was landed with ongoing or failed builds.Dec 17 2020, 8:33 AM
This revision was automatically updated to reflect the committed changes.