[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 inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:250
+	llvm::ArrayType *KmpCriticalNameTy;
+	llvm::PointerType *KmpCriticalNamePtrTy;
+
----------------
fghanim wrote:
> 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.
These are not good arguments. We have *all other types* defined in `OMPKinds.def` + `OMPConstants.{h/cpp}`, doing something different here just complicates the code.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:259
+
+public:
+
----------------
fghanim wrote:
> 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.
It is futile to design based on some use cases we cannot describe properly. Once the need arises for public interfaces, the `callType` (inconsistent spelling!), the `Entry/Exit` call stuff, we can add it but not before.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:646
+
+	Directive OMPD = Directive::OMPD_master;
+	Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
----------------
fghanim wrote:
> jdoerfert wrote:
> > You can just write `OMPD_master` below instead of `OMPD`.
> Yes. I just feel this is a bit cleaner is all.
I argue it is harder to read the code this way (especially at the call site below). Though, I won't make the change a requirement.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:850
+	return RTLFunc;
+}
+
----------------
fghanim wrote:
> 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 :)
What is `EmitCommonDirectiveEntry` and `EmitCommonDirectiveExit` ?

I don't see how it is helping to have `emitEntry(createEntry(DIRECTIVE))` instead of emit(`DIRECTIVE_ENTRY`), especially since the former needs all this logic to map `DIRECTIVE` to `DIRECTIVE_ENTRY`. The latter shows you at the call site, e.g., in `CreateCritical`, exactly what runtime call is being created and at these call sites we already know exactly what runtime call we need. 

You can still have some helper that puts entry/exit around some code, simply pass both the entry and exit runtime call ID instead of just the directive ID from which you would "compute" the entry/exit runtime call ID.

---

Long story short, if making things explicit minimizes code complexity along the way I'm all for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72304