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

[Fady Ghanim via Phabricator via llvm-commits]

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