[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Fady Ghanim via Phabricator via llvm-commits]

fghanim marked 3 inline comments as done.
fghanim added a comment.

In D79675#2037314 <https://reviews.llvm.org/D79675#2037314>, @jdoerfert wrote:

> I left some comments on the type stuff. The rest looks good.
>  I think if you rebase the type stuff on D79739 <https://reviews.llvm.org/D79739> (which I can merge) we should only need to expand `initializeTypes` to make this work as expected. WDYT?


Currently, I implemented the changes relevant to me from that and made a separate local commit prior to this one to make sure things work.
I build llvm locally, and so it take 6 - 8 hours, so when all patches are accepted, I'll do a rebase, etc. in one go to make sure there are no problems.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:234
 __OMP_RTL(__kmpc_critical, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy)
-__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy, Int32)
+__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy, IntPtrTy)
 __OMP_RTL(__kmpc_end_critical, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy)
----------------
jdoerfert wrote:
> fghanim wrote:
> > jdoerfert wrote:
> > > fghanim wrote:
> > > > jdoerfert wrote:
> > > > > I think this was correct before:
> > > > > ```
> > > > >   KMP_EXPORT void __kmpc_critical_with_hint(ident_t *, kmp_int32 global_tid, kmp_critical_name *, uint32_t hint);
> > > > > ```
> > > > Nop, it's supposed to be whatever `IntPtrTy` is in the frontend (i.e. 32 for 32bit, 64 for 64bit).
> > > > 
> > > > `IntPtrTy` is actually a union with `sizety` in `CodeGenModule`
> > > I'm confused. I copied the declaration above from the runtime. It doesn't contain an `IntPtrTy` or similar. I agree that `IntPtrTy` is machine dependent and we need your initializers. What I tried to say is that at least one declaration in the runtime has `__kmpc_critical_with_hint` with an int32 as last argument. That said, the runtime is not shy of incompatible declarations for functions.
> > I cannot speak for what's in the runtime. However, in clang, this currently is defined to use `IntPtrTy`. If you go check, I have a todo in the current implementation for critical to come back and fix it.
> That is just an existing bug in Clang. The runtime is consistent with the type and arguably it is the runtime we must match, not the other way around ;)
Apparently, this used to be `uintptr_t` in the runtime and was changed by D51235 . It doesn't say why the change was made.

Clang uses this in multiple places, which makes me think it may have been intentional. So I suggest we go by the standard. Where can I find what does the OMP standard say about how the signature of the runtime calls should look like? This way I'll fix this one accordingly, and later we may go and make sure that all the rest match up with the standard - where possible.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:69
 
 void llvm::omp::types::initializeTypes(Module &M) {
   if (Void)
----------------
jdoerfert wrote:
> Maybe we can/should take the IntPtr and SizeTy here so there is only a single call necessary that initializes all types. I fear "default" values are problematic.
> 
> WDYT?
you mean pass it as a second argument? Sure, I would love that. I mean it's fine with me either way. I originally wanted to do that. but then thought maybe it is slightly better to explicitly initialize target specific data types, to give initialize type a cleaner look ... maybe?!


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:106
+  initializeVoidPtrTy(VoidPtrTy);
+}
+
----------------
jdoerfert wrote:
> I guess we can directly call the initialize functions, right?
> With an assert that SizeTy is set we can make sure users do it ;)
Sure, then again, I am just following in the stylistic footsteps of the pioneers who started the OMPBuilder, who created an initialize for initialize ;)

On second thought, I think I will just create something like `initializeTargetSpecificTypes()` sort of thing, and be done with it. instead of having one for each type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675