This is an archive of the discontinued LLVM Phabricator instance.

[openmp] Remove clause from OMPKinds.def and use OMP.td info
ClosedPublic

Authored by clementval on Dec 9 2020, 11:21 AM.

Details

Summary

Remove the OpenMP clause information from the OMPKinds.def file and use the
information from the new OMP.td file. There is now a single source of truth for the
directives and clauses.

To avoid generate lots of specific small code from tablegen, the macros previously
used in OMPKinds.def are generated almost as identical. This can be polished and
possibly removed in a further patch.

Diff Detail

Event Timeline

clementval created this revision.Dec 9 2020, 11:21 AM
clementval requested review of this revision.Dec 9 2020, 11:22 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 9 2020, 11:22 AM

Remove useless define macro

I like this a lot. We might even look into generating the clang OpenMPClause classes via TableGen later, thanks a lot.

There is a file missing, right? The list of actual clauses.

clementval added a comment.EditedDec 9 2020, 11:55 AM

I like this a lot. We might even look into generating the clang OpenMPClause classes via TableGen later, thanks a lot.

There is a file missing, right? The list of actual clauses.

The list of actual clauses is already in and used by Flang (llvm/include/llvm/Frontend/OpenMP/OMP.td). Or do you think about another list?

jdoerfert accepted this revision.Dec 9 2020, 12:13 PM

LGTM.

(I missed the part that we duplicated the information so far)

This revision is now accepted and ready to land.Dec 9 2020, 12:13 PM
This revision was landed with ongoing or failed builds.Dec 10 2020, 7:19 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Dec 10 2020, 7:21 AM
thakis added inline comments.
clang/include/clang/AST/ASTTypeTraits.h
152

Now we're even including the cpp.inc file in .h files. Please consider following the more regular way of doing .td stuff I've outlined in D83636.

And a less philosophical comment: This causes a bunch of Wcovered-switch-default warnings:

../../clang/include/clang/AST/OpenMPClause.h:7777:5: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
../../clang/include/clang/AST/RecursiveASTVisitor.h:2962:3: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]

(and possibly more).

And a less philosophical comment: This causes a bunch of Wcovered-switch-default warnings:

../../clang/include/clang/AST/OpenMPClause.h:7777:5: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
../../clang/include/clang/AST/RecursiveASTVisitor.h:2962:3: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]

(and possibly more).

Yeah just saw that and reverted the patch. I'll rework a bit the patch to include your suggestion from D83636. MLIR is doing it this way so I'm fine with doing it the other way as well.

clementval reopened this revision.Dec 10 2020, 8:15 AM
This revision is now accepted and ready to land.Dec 10 2020, 8:15 AM
clementval updated this revision to Diff 310980.EditedDec 10 2020, 12:22 PM

Fix -Werror=covered-switch-default problems + rename OMP.cpp.inc to OMP.inc since the .cpp does
not make sense anymore.

@jdoerfert Does it still work for you?

@thakis I have renamed the file included since it can be included in header file or cpp file.

Ping since there was a phab email delivery problem.

jdoerfert accepted this revision.Dec 16 2020, 8:49 AM

LGTM still

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