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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Thanks for these! We should probably split this in three patches.  I commented below, mostly minor stuff.



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


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:68
+  InsertPointTy SaveIP() { return Builder.saveIP(); }
+
   /// Callback type for variable finalization (think destructors).
----------------
The above can be committed separately as "wrap IRBuilder methods" or something, LGTM for that part.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:371
+  /// threads copy the 'copyin' variables from Master copy to threadprivate
+  /// copies.
+  ///
----------------
Copy & paste, also below.


================
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)
----------------
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);
```


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


================
Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:122
+  }
+}
+
----------------
(I doubt we need the `initVoidPtr` function but if we do, the condition looks wrong)


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:594
+  //         PRegPreFiniTI->getSuccessor(0) == PRegExitBB &&
+  //         "Unexpected CFG structure!");
 
----------------
Preferably we would have a modified assertion. If that is too cumbersome, we can probably remove it. 


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:930
+    InsertPointTy IP, Value *MasterAddr, Value *PrivateAddr,
+    llvm::IntegerType *IntPtrTy, bool BranchtoEnd) {
+  if (!IP.isSet())
----------------
Can you add a comment here, maybe some ascii art, describing what we generate.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1013
+  return Builder.CreateCall(Fn, Args);
+}
+
----------------
I think we should have a unit test for each of these.

Style: drop the `llvm::`, not needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675