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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D79675#2032764 <https://reviews.llvm.org/D79675#2032764>, @fghanim wrote:

> This is a small patch as it is. splitting it any further would make it very very small :D


Very small is great, then I can accept it fast, finding enough time to read larger patches is part of why it takes me so long to do reviews.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:101
+extern Type *IntPtrTy;
+extern Type *SizeTy;
+
----------------
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.


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


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


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1013
+  return Builder.CreateCall(Fn, Args);
+}
+
----------------
fghanim wrote:
> jdoerfert wrote:
> > I think we should have a unit test for each of these.
> > 
> > Style: drop the `llvm::`, not needed.
> Aside from `CreateCopyinclauseblocks()`, I couldn't think of a unittest for the others, all they do is create a runtime call based on the passed arg. That's why I didn't do it already.
Let's test that one then :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675