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

[Fady Ghanim via Phabricator via llvm-commits]

fghanim marked 7 inline comments as done.
fghanim added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:101
+extern Type *IntPtrTy;
+extern Type *SizeTy;
+
----------------
jdoerfert wrote:
> jdoerfert wrote:
> > I can totally see why we need `int` and `size_t` but I'm not sure about the others.
> > `void` is universally translated to `i8*` Adding a pointer to a type should be done in OMPKinds.def, something like:
> > ```
> > #define __OMP_PTR_TYPE(NAME, BASE) OMP_TYPE(NAME, Base->getPointerTo());
> > __OMP_PTR_TYPE(VoidPtr, Int8)
> > __OMP_PTR_TYPE(VoidPtrPtr, VoidPtr)
> > ```
> My proposed scheme for the pointers was integrated in D79739 and will be commited shortly.
Kept a couple a Ineed in OMPConstants. will add them in OMPKinds.def after the other patch lands.


================
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:
> > > 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.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:240
+__OMP_RTL(__kmpc_threadprivate_cached, false, VoidPtr, IdentPtr, Int32, VoidPtr, SizeTy, Void3Ptr)
+__OMP_RTL(__kmpc_threadprivate_register, false, Void, IdentPtr, VoidPtr, KmpcCtorTy, KmpcCopyCtorTy, KmpcDtorTy)
+
----------------
jdoerfert wrote:
> Can you add attributes (below) for these and call them in `llvm/test/Transforms/OpenMP/add_attributes.ll` [if not already present]. These changes and the additional types should go in a separate patch.
Added in D79739


================
Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:122
+  }
+}
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675