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

[Fady Ghanim via Phabricator via cfe-commits]

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

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

> What's the status? Can we split the target specific types stuff if this may take a while, other patches depend on that :)


Could you please list the other patches that are being held back by this one? I'd be interested to have a look at them. :)



================
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:
> > > > > 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.
> This is not a standard function. It is a runtime function. I suspect it is a int32_t because enums default to `int` which is 32 bit on all interesting platforms. Should probably be a `int` but I would need to read up to confirm. Long story short, and as I said before, it is a bug in clang. The code here was correct.
> 
> FWIW, we will soon replace clang's runtime function generation with these macros. That will happen in the next days I hope.
Oh, my bad. I thought OMP runtime functions are designed a specific way based on the OMP standard. If that's not the case, then it doesn't matter. I'll drop this change as well. There is a handful of hints anyway.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:122
+  }
+}
+
----------------
jdoerfert wrote:
> fghanim wrote:
> > jdoerfert wrote:
> > > fghanim wrote:
> > > > jdoerfert wrote:
> > > > > (I doubt we need the `initVoidPtr` function but if we do, the condition looks wrong)
> > > > Forgot to refractor when I changed names.
> > > > 
> > > > Regarding `void*` , I am not sure how it is defined in fortran. That's why I made it possible to initialize it separately.
> > > I see. Let's cross that bridge once we get to it.
> > Modified it a bit and removed usage from initialization. If fortran people don't end using it, then we can remove it.
> Let's not add stuff that might be used.
I just looked this up. Apparently, Fortran doesn't seem to have the concept of void pointer natively. However, They have something called `Assumed types` , which was added for interoperability with C, which *probably* means it will follow whatever C does to handle void pointers.

I'll remove it now, and if they choose, people can add it later.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:106
+  initializeVoidPtrTy(VoidPtrTy);
+}
+
----------------
jdoerfert wrote:
> fghanim wrote:
> > 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.
> Point taken, though I'm fine with criticizing my own design :)
> 
> My suggestion now: merge everything into the `OpenMPIRBuilder::initialize` we already have. I'm also fine with removing this and letting users call (a single) `initializeTypes`, thought it is a bit harder to "know" that is needed.
> 
> 
I think either way is fine, so I wasn't criticizing. I was explaining why I did what I did. :)

Anyways, I made the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675