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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

typo in the title.



================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.h:1444
+    /// alloca address where the runtime returns the device pointers.
+    llvm::DenseMap<const ValueDecl *, llvm::Value *> CaptureDeviceAddrMap;
   };
----------------
If it is an alloca (for sure) use `llvm::AllocaInst`.


================
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:%.+]],
----------------
Did we just change the order or is this non-deterministic? The latter is not acceptable, the former might be.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4131
+      for (auto DeviceMap : Info.DevicePtrInfoMap) {
+        if (isa<AllocaInst>(DeviceMap.second.second)) {
+          auto *LI =
----------------
What else could it be here?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4694
+      } else if (CombinedInfo.DevicePointers[I] == DeviceInfoTy::Address) {
+        Info.DevicePtrInfoMap[BPVal] = {BP, BP};
+        assert(DeviceAddrCB &&
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152554