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

[Fady Ghanim via Phabricator via cfe-commits]

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

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



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


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


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:594
+  //         PRegPreFiniTI->getSuccessor(0) == PRegExitBB &&
+  //         "Unexpected CFG structure!");
 
----------------
jdoerfert wrote:
> Preferably we would have a modified assertion. If that is too cumbersome, we can probably remove it. 
I was going to remove it actually. When running Dtor over an array, clang emits some loop logic that makes this untrue.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1013
+  return Builder.CreateCall(Fn, Args);
+}
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675