[PATCH] D152554: [OpenMP] Migrate deviice code privatization from Clang CodeGen to OMPIRBuilder

[Akash Banerjee via Phabricator via llvm-commits]

TIFitis added inline comments.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.h:1444
+    /// alloca address where the runtime returns the device pointers.
+    llvm::DenseMap<const ValueDecl *, llvm::Value *> CaptureDeviceAddrMap;
   };
----------------
jdoerfert wrote:
> If it is an alloca (for sure) use `llvm::AllocaInst`.
It's alloca for use_dev_ptr and usually GEPInst for use_dev_addr.
Updated comment to llvm address


================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:7170-7234
+    const llvm::Value *DevPtr = nullptr;
 
     // In order to identify the right initializer we need to match the
     // declaration used by the mapping logic. In some cases we may get
     // OMPCapturedExprDecl that refers to the original declaration.
-    const ValueDecl *MatchingVD = OrigVD;
-    if (const auto *OED = dyn_cast<OMPCapturedExprDecl>(MatchingVD)) {
+    if (const auto *OED = dyn_cast<OMPCapturedExprDecl>(OrigVD)) {
       // OMPCapturedExprDecl are used to privative fields of the current
----------------
TIFitis wrote:
> Currently clang maintains a map between Clang::ValueDecl and corresponding address in Info.CaptureDeviceAddrMap.
> 
> Please see the EmitOMPUseDeviceAddrClause function just below this to see how it does so.
> 
> I would however like to get rid of this map as it allows us to then get rid of another map in the MapInfosTy and a clang specific callback for maintaining these maps..
> 
> This map is only used to retain the mapping between a ValueDecl inside a map clause to its corresponding llvm address.
> 
> The highlighted code here tries to retrieve this llvm address back from the ValueDecl without using the map, but as you can it is a very hacky way of doing so. But my intuition says that there must be a much simpler method of retrieving this address as Clang must be storing it in some way in order for it to generate code.
> 
> It would be great if someone with more experience in Clang codegen could help me achieve this. 
> 
> Also I am aware that I am probably doing a very poor job at explaining this. Please let me know if you have any questions or would like me to try and explain it with more clarity.
Please ignore the above comment. I've kept things simpler for this patch.

I'll try to have another patch in the future to focus on removing the clang specific maps and callbacks.


================
Comment at: clang/test/OpenMP/target_data_use_device_ptr_codegen.cpp:418
-    // CK2:     [[VAL1:%.+]] = load ptr, ptr [[BP1]],
-    // CK2:     store ptr [[VAL1]], ptr [[PVT1:%.+]],
     // CK2:     store ptr [[PVT1]], ptr [[_PVT1:%.+]],
----------------
jdoerfert wrote:
> Did we just change the order or is this non-deterministic? The latter is not acceptable, the former might be.
It's a change of order. I've used a `llvm::SmallMapVector` for determinism.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4131
+      for (auto DeviceMap : Info.DevicePtrInfoMap) {
+        if (isa<AllocaInst>(DeviceMap.second.second)) {
+          auto *LI =
----------------
jdoerfert wrote:
> What else could it be here?
For use_dev_ptr a new alloca is inserted.
For use_dev_addr it points to the GEP instruction from the mappers.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4694
+      } else if (CombinedInfo.DevicePointers[I] == DeviceInfoTy::Address) {
+        Info.DevicePtrInfoMap[BPVal] = {BP, BP};
+        assert(DeviceAddrCB &&
----------------
jdoerfert wrote:
> What if the PointerBCOrASCast and the GEP are both no-ops, and BPVal is an alloca. Would that cause a problem as we later only check if the type of the value is an alloca. That said, it seems dangerous to only rely on the type of the value.
I don't think the GEP can be  a no-op here as it's from the mapper array that we are adding above.

That being said, at worst we would be emitting a spurious load and store instruction with no use.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152554