[PATCH] D72304: [OpenMP]{OMPIRBuilder] Add Directives (master and critical) to OMPBuilder.

[Johannes Doerfert via Phabricator via llvm-commits]

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