This is an archive of the discontinued LLVM Phabricator instance.

[Flang][OpenMP] Semantic checks for Atomic construct.
ClosedPublic

Authored by sameeranjoshi on Oct 16 2020, 12:08 PM.

Details

Summary

Patch implements restrictions from 2.17.7 of OpenMP 5.0 standard for atomic Construct. Tests for the same are added.

One of the restriction
OpenMP constructs may not be encountered during execution of an atomic region.
Is mentioned in 5.0 standard to be a semantic restriction, but given the stricter nature of parser in F18 it's caught at parsing itself.

This patch is a next patch in series from D88965.

Diff Detail

Event Timeline

sameeranjoshi created this revision.Oct 16 2020, 12:08 PM
Herald added a project: Restricted Project. · View Herald Transcript
sameeranjoshi requested review of this revision.Oct 16 2020, 12:08 PM
clementval added inline comments.Oct 16 2020, 5:08 PM
flang/test/Semantics/omp-atomic01.f90
5

Regarding your general comment. hint does not have a parser::Hint since it is in the OmpClause variant. So you should be able to resolve this todo in this patch as well.

sameeranjoshi edited the summary of this revision. (Show Details)
sameeranjoshi added a project: Restricted Project.

Resolves the TODO, by fixing a small bug for Hint.
Adds more test cases for At most one hint clause may appear on the construct.

sameeranjoshi marked an inline comment as done.Oct 20 2020, 1:44 AM
sameeranjoshi added inline comments.
flang/test/Semantics/omp-atomic01.f90
5

Thank you @clementval.
I have resolved the TODO.
OMPC_Hint inside OMP.td was supposed to use flangClassValue instead of flangClass.

clementval added inline comments.Oct 21 2020, 11:27 AM
flang/lib/Semantics/check-omp-structure.cpp
10

Is this necessary?

486

Why is there a specific check for this? If it is not allowed it should not be in OMP.td and then catch be CheckAllowed in enter acquire.

490

Same here. This should be catch be the generic code and the TableGen map.

flang/lib/Semantics/check-omp-structure.h
162

These are not really clause but atomic constructs.

clementval requested changes to this revision.Oct 21 2020, 11:27 AM
This revision now requires changes to proceed.Oct 21 2020, 11:27 AM
sameeranjoshi marked an inline comment as done.Oct 21 2020, 11:36 AM
sameeranjoshi added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
10

For common::visitors I will see of they can be removed.

486

Standard says:
If atomic-clause is update or not present then memory-order-clause must not be acq_rel or acquire.

I couldn't use CheckNotAllowedIfClause here reason being there is no OMPC_atomic.
I see OMPC_atomic_default_mem_order but I doubt it's the same what I was trying to find.

also aquire acq_rel are not allowed only here they might be allowed somewhere else, hence changing them in OMP.td would affect other atomic directives as well.

clementval added inline comments.Oct 21 2020, 11:43 AM
flang/lib/Semantics/check-omp-structure.cpp
486

Then the check for update is missing? and it will fail for atomic read even it should be allowed?

flang/lib/Semantics/check-omp-structure.h
155

Thinking about this again. It will be good if we can retain memory-order-clause in the parse tree. Since the standard specifically talks about memory-order-clauses. If we write a frontend tool to list all memory order clauses then if there is a representation for memory-order-clause in the parse tree it will be easy to process all memory-order-clauses.

sameeranjoshi added inline comments.Oct 22 2020, 7:30 AM
flang/lib/Semantics/check-omp-structure.h
155

The whole discussion to change parser started when failing to implement semantic checks(given that we had code) without changing parser.

Reason being that memory-order-clauses were not part of OmpClause inside openmp-parsers.cpp.

I am happy to change it If that's possible.
Also if @clementval shares some opinions on this issue, he might have a better overview.

clementval added inline comments.Oct 22 2020, 8:30 AM
flang/lib/Semantics/check-omp-structure.h
155

Well, I don't have a strong opinion on that but if you want to use the directive-clauses map generated by TableGen you need to have clauses in OmpClause.

I think there could be a way to keep the clauses in OmpClause and have a separate parser that handle the memory-order-clauses and that could be applied to Atomic for example.

I guess you need to rebase this now that the memory-order-clause is in.

Changes in atomic to use OmpAtomicClauseList after D91839 ( memory-order-clause related).

sameeranjoshi marked 4 inline comments as done and an inline comment as not done.Nov 27 2020, 4:20 AM
sameeranjoshi added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
486

Why is there a specific check for this?

Due to missing clause for ? inside below function which isn't in OMP.td.

CheckNotAllowedIfClause( ? ,
      {llvm::omp::Clause::OMPC_acquire, llvm::omp::Clause::OMPC_acq_rel})

Reason being that there is nothing mentioned inside TableGen.

If it is not allowed it should not be in OMP.td and then catch be CheckAllowed in enter acquire.

If we remove it from OMP.td that would be not found for other atomic clauses where it was expected to work.

486

Then the check for update is missing?

umm... update check was added and I see that we have
test with comment ! 2.17.7.6 which works well due to CheckNotAllowedIfClause which fits for llvm::omp::Clause::OMPC_update

I might not be still clear on how to leverage TableGen for this particular test case.
I am happy to use TableGen wherever possible.
I even tried to add OMPC_Atomic but that's not helping.

flang/lib/Semantics/check-omp-structure.h
155

Handled by D91839.

sameeranjoshi marked an inline comment as done.Dec 8 2020, 6:24 AM

Ping.

clementval added inline comments.Dec 8 2020, 8:15 AM
flang/include/flang/Parser/parse-tree.h
3634

Is the comment at the right place?

flang/lib/Semantics/check-omp-structure.cpp
487

Where do you check that it is only for update?

sameeranjoshi added inline comments.Dec 8 2020, 8:44 AM
flang/include/flang/Parser/parse-tree.h
3634

No, thx.

flang/lib/Semantics/check-omp-structure.cpp
487

I check separately for both
If atomic-clause is update or not present then memory-order-clause must not be acq_rel or acquire.
break into below
restriction:

If atomic-clause is update then memory-order-clause must not be acq_rel or acquire.

in
void OmpStructureChecker::Leave(const parser::OmpAtomicUpdate &)
&

If atomic-clause is not present then memory-order-clause must not be acq_rel or acquire.

in this Leave.

clementval accepted this revision.Dec 8 2020, 12:50 PM

Looks ok to me. Just a small comment on the OmpAtomic leave

flang/lib/Semantics/check-omp-structure.cpp
487

Ok I see. The OmpAtomic is the atomic construct without clause? If so, maybe adding small comment here to precise this would be nice.

This revision is now accepted and ready to land.Dec 8 2020, 12:50 PM
sameeranjoshi marked 5 inline comments as done.Dec 10 2020, 12:45 AM

Thank you for reviewing.

flang/lib/Semantics/check-omp-structure.cpp
487

Ok I see. The OmpAtomic is the atomic construct without clause? If so, maybe adding small comment here to precise this would be nice.

To be specific it's not a general clause but called as 'atomic-clause' in standard.
Which has values read/write/update/capture.

To answer the question - yes it doesn't have 'atomic-clause'.

!$omp atomic [clause[[,] clause] ... ] <no atomic clause here>
     update-statement
[!$omp end atomic]
clementval added inline comments.Dec 10 2020, 7:17 AM
flang/lib/Semantics/check-omp-structure.cpp
487

Yeah I get it. Since other "with" clause are represented by a node as well like OmpAtomicUpdate I still think it would make sense to have a small comment here to mentioned that the OmpAtomic node represent and atomic directive without atomic-clause

sameeranjoshi marked an inline comment as done.

Add a clarification comment.

Add a clarification comment.

Thanks for adding this. Looks good to land from my side

This revision was landed with ongoing or failed builds.Dec 13 2020, 11:35 PM
This revision was automatically updated to reflect the committed changes.
sameeranjoshi marked an inline comment as done.Dec 13 2020, 11:59 PM