The new OpenMPConstants.h is a location for all OpenMP related constants
(and helpers) to live.
This patch moves the directives there (the enum OpenMPDirectiveKind) and
rewires Clang to use the new location.
Initially part of D69785.
Paths
| Differential D69853
[OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h ClosedPublic Authored by jdoerfert on Nov 5 2019, 8:40 AM.
Details Summary The new OpenMPConstants.h is a location for all OpenMP related constants This patch moves the directives there (the enum OpenMPDirectiveKind) and Initially part of D69785.
Diff Detail
Event TimelineHerald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 5 2019, 8:40 AM
jdoerfert added inline comments.
jdoerfert marked 5 inline comments as done. Comment ActionsAddressed review comments, simplified some changes, moved file, ... Comment Actions Again, better to split into 2 patches, one for LLVM and one for clang.
jdoerfert added inline comments.
Comment Actions
I split the other patch, but this one cannot be split in a reasonable (=testable) manner. Splitting a patch only to have smaller diffs is not the goal. Comment Actions
We don't need to test it. Just split it into 2 parts, llvm and clang, nothing else.
Meinersbur added inline comments.
jdoerfert added inline comments.
jdoerfert added a child revision: D69785: [OpenMP] Introduce the OpenMP-IR-Builder.Nov 6 2019, 9:32 PM Comment Actions Are there blocking issues on this one or can we go ahead and adjust minor details as we go? Comment Actions
I still think it would be good to separate patches into the LLVM part and into the clang part and commit them separately? E.g. flang people may try to look for the commits for constant in LLVM, but not for the clang changes. It will be much easier for them to skip the changes in clang.
Comment Actions
But they might also want to see how to interact with the constants so having the clang parts in there is good. I do not see a clear benefit, e.g., Flang ppl might or might not prefer a separate patch, but I see a clear drawback (testing wise) so I still don't believe splitting the right thing.
jdoerfert marked an inline comment as done. Comment ActionsUse an llvm::IndexedMap instead of std::map Comment Actions
Testing is not a problem for NFC LLVM part of the patch, I think. Plus, this whole patch is NFC.
Comment Actions
That is not a reason not to test it. And again, there are arguments for a combined patch even beyond testing.
Comment Actions LG with a nit
This revision is now accepted and ready to land.Nov 7 2019, 12:06 PM jdoerfert marked 2 inline comments as done. Comment ActionsCreate llvm/libFrontend, move code there, make shared library builds work. llvm/IR/ is an unfortunate location as we cannot use llvm utility functions The new libFrontend should not only house the OpenMP-IR-Builder but other code fpetrogalli added inline comments.
jdoerfert added inline comments.
Comment Actions Build result: pass - 60659 tests passed, 0 failed and 726 were skipped. Log files: console-log.txt, CMakeCache.txt Closed by commit rGeb3e81f43f01: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h (authored by jdoerfert). · Explain WhyDec 9 2019, 10:19 PM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 233003 clang/include/clang/AST/OpenMPClause.h
clang/include/clang/AST/StmtOpenMP.h
clang/include/clang/Basic/OpenMPKinds.h
clang/include/clang/Basic/OpenMPKinds.def
|
I am not a fan of using in header files in general contexts. It's mostly a stylistic preference. Why not use llvm::omp::Directive everywhere? It is not much longer than the substitution...