jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.
LGTM with some minor things you should address before the merge.
================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:90
+// extern ArrayType *KmpCriticalNameTy;
+// extern PointerType *KmpCriticalNamePtrTy;
+
----------------
Leftover comment.
================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:272
+ /// \param CriticalName name of the lock used by the critical directive
+ /// \param hasHint whether there is ahint clause associated with critical
+ ///
----------------
Nit: typos in parameter name and comment, comments seems outdated.
================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:305
+ /// should be
+ /// called.
+ ///
----------------
Nit: Comment is not in sync with the Entry one.
Nit: spelling in the parameter name and format.
================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:326
+ /// be
+ /// called.
+ ///
----------------
Nit: spelling param name and format.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:45
///{
-
#define OMP_TYPE(VarName, InitValue) Type *llvm::omp::types::VarName = nullptr;
----------------
Leftover
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:695
+
+ FinalizationStack.push_back({FiniCB, OMPD, /*IsCancellable*/ false});
+
----------------
This needs to be guarded by `HasFinalize` if the corresponding pop is.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:749
+ BasicBlock *ExitPredBB = SplitPos->getParent();
+ auto InsertBB = merged ? ExitPredBB : ExitBB;
+ if (!isa_and_nonnull<BranchInst>(SplitPos))
----------------
I have the feeling the merging makes it harder without providing a benefit but
I'm OK with it for now
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:759
+OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::emitCommonDirectiveEntry(
+ Directive omp, Value *EntryCall, BasicBlock *ExitBB, bool Conditional) {
+
----------------
`omp` is not a good name here.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:791
+ omp::Directive OMPD, InsertPointTy FinIP, Instruction *ExitCall,
+ bool hasFinalize) {
+
----------------
`HasFinalize`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72304/new/
https://reviews.llvm.org/D72304
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
