Page MenuHomePhabricator

[flang][openmp] Check clauses allowed semantic with tablegen generated map
ClosedPublic

Authored by clementval on Jul 7 2020, 10:37 AM.

Details

Summary

This patch is enabling the generation of clauses enum sets for semantics check in Flang through
tablegen. Enum sets and directive - sets map is generated by the new tablegen infrsatructure for OpenMP
and other directive languages.
The semantic checks for OpenMP are modified to use this newly generated map.

Diff Detail

Event Timeline

clementval created this revision.Jul 7 2020, 10:37 AM
clementval marked an inline comment as done.Jul 7 2020, 10:39 AM
clementval added inline comments.
flang/test/Semantics/omp-clause-validity01.f90
461

As a side note, This is supposed to be fine in Clang so I removed the check. I looked at the OpenMP 5.0 std and didn't see a restriction on reduction for task loop simd.

clementval retitled this revision from [flang][openmp] Check clauses allowance with tablegen generated map to [flang][openmp] Check clauses allowed semantic with tablegen generated map.Jul 7 2020, 10:39 AM
ichoyjx added inline comments.Jul 9 2020, 9:58 PM
flang/test/Semantics/omp-clause-validity01.f90
461

What's the current plan? Are we trying to cover OpenMP 5.0 Spec for semantics (it appears so)?

llvm/include/llvm/Frontend/OpenMP/OMP.td
230

Bear with me, what does 50 mean?

431–435

For target enter and target exit, nowait is only allowed once. If it's allowed here, will this restriction be captured by the rules in target directive above?

clementval marked 6 inline comments as done.

Address review comment about NoWait in target eneter/exit data

Thanks for the review. I just updates the patch and answered your questions.

flang/test/Semantics/omp-clause-validity01.f90
461

Clang just moved to 5.0as default and my guesses that we are targeting 5.0 as well since it is the current standard.

llvm/include/llvm/Frontend/OpenMP/OMP.td
230

The VersionedClause is defined as this: VersionedClause<VersionedClause<Clause c, int min = 1, int max = 0x7FFFFFFF>

So here it means the clause is valid from version 5.0 and up. This is currently not used in Flang but in Clang it's used and I took the value from the old macros definition OMPKinds.def. So version 4.5 would 45 and so on.

431–435

Good catch. I updated the patch and moved them in the correct set.

jdoerfert added inline comments.Jul 10 2020, 4:00 PM
flang/lib/Semantics/check-omp-structure.cpp
67

I love these patches :) And we clean up Clang after!

flang/test/Semantics/omp-clause-validity01.f90
461

If Flang has the equivalent to -fopenmp-version=XX or you just hardcode a version to be used, it should be possible to retain this error for the 4.5 case, right?

llvm/include/llvm/Frontend/OpenMP/OMP.td
217

Yay! :)

230

FWIW, that is how we specify the version on the command line (for clang) as well.

llvm/utils/TableGen/DirectiveEmitter.cpp
461

Any reason this is flang specific? I guess we want to use the information in Clang too (soonish).

clementval marked 7 inline comments as done.Jul 10 2020, 4:04 PM
clementval added inline comments.
flang/test/Semantics/omp-clause-validity01.f90
461

Flang does not support passing a version at the moment. So we have to assume it's 5.0. I guess there will be work work on this later.

llvm/utils/TableGen/DirectiveEmitter.cpp
461

No specific reason. I'm happy to remove the FLANG_ here.

ichoyjx accepted this revision.Jul 10 2020, 5:10 PM

Thanks for the Nowait changes.

Very nice work!

This revision is now accepted and ready to land.Jul 10 2020, 5:10 PM
DavidTruby accepted this revision.Jul 10 2020, 6:29 PM

This is great! Much neater than what I wrote for the combined constructs before and sharable with the rest of llvm :)
Thanks!

clementval marked 2 inline comments as done.

Rebase

This revision was automatically updated to reflect the committed changes.