fghanim marked 5 inline comments as done.
fghanim added a comment.
I don't think there is enough in this patch to split it into two or three
patches. The main part is `EmitOMPInlinedRegion` which does all the heavy
lifting. At this point, both create `Master` & `Critical` are almost wrappers
collecting arguments for `EmitOMPInlinedRegion`. So I really do not want to
split them into multiple patches unless I have to.
As for the rest , I just wanted to provide my thinking when I did them. I'll
update them if you think it's better to. let me know.
================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:250
+ llvm::ArrayType *KmpCriticalNameTy;
+ llvm::PointerType *KmpCriticalNamePtrTy;
+
----------------
jdoerfert wrote:
> 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.
I tried to do that, but started to get compilation errors when I put them in
`OMPConstants.{h/cpp}` . So I thought that given that these are used exactly
once according to current implementation in clang (i.e. for 'critical name').
I'll do it as part of OMPBuilder for the time being.
================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:259
+
+public:
+
----------------
jdoerfert wrote:
> A lot of the functions below should not be public. We should expose as little
> as possible, mainly the `CreateDirective` methods.
I think the OMPbuilder should provide people writing llvm transformation passes
(e.g. autoparallelizing) with a way to be able to emit correct OMP calls based
on their needs.
If you have minor suggestions to make it better, I'd love to update this based
on that.
Alternatively, I'll make them private for now, until we decide on something for
that.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:646
+
+ Directive OMPD = Directive::OMPD_master;
+ Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
----------------
jdoerfert wrote:
> You can just write `OMPD_master` below instead of `OMPD`.
Yes. I just feel this is a bit cleaner is all.
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:656
+
Args, /*Conditional*/true, /*hasFinalize*/true);
+}
+
----------------
jdoerfert wrote:
> Make sure the patch is clang formatted. See also:
> https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting
Will do
================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:850
+ return RTLFunc;
+}
+
----------------
jdoerfert wrote:
> 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?
While I completely agree that the switch+if-cascade is not an elegant solution,
It's helpful in enabling a generic way to the directive entry and exit calls.
which is in turn helpful for `EmitCommonDirectiveEntry` and
`EmitCommonDirectiveExit`.
FWIW, `CreateEntryCall` was used inside of `EmitCommonDirectiveEntry`, similar
to how `CreateExitCall` is currently inside `EmitCommonDirectiveExit`.
`RTLFuncName` enabled that. I only took it out because of Critical's
`Entry_Hint` special case. I plan to put it back in, If critical is the only
directive like that.
I am not too attached to it, but I think it's help-ing :)
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
