[PATCH] D72304: Summary: Add OpenMP Directives (master and critical) to OMPBuilder, and use them in clang.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Some initial comments. Can we split it in two patches (master/critical)?



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:250
+	llvm::ArrayType *KmpCriticalNameTy;
+	llvm::PointerType *KmpCriticalNamePtrTy;
+
----------------
If there is no good reason agains it, these should go into the `OMPKinds.def`/`OMPConstants.{h/cpp}`. We need support for array types but that is not a problem. 


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:259
+
+public:
+
----------------
A lot of the functions below should not be public. We should expose as little as possible, mainly the `CreateDirective` methods.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:646
+
+	Directive OMPD = Directive::OMPD_master;
+	Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
----------------
You can just write `OMPD_master` below instead of `OMPD`.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:656
+													Args, /*Conditional*/true, /*hasFinalize*/true);
+}
+
----------------
Make sure the patch is clang formatted. See also: https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:850
+	return RTLFunc;
+}
+
----------------
Is this switch + if-cascade + `callEntry/Exit` really helpful?
I mean we can replace
  `Instruction *EntryCall = CreateEntryCall(OMPD, Args);`
with
  `Instruction *EntryCall = Builder.CreateCall(getOrCreateRuntimeFunction(OMPRTL___kmpc_omp_master), Args);`
right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72304/new/

https://reviews.llvm.org/D72304