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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
