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