[PATCH] D146648: [MLIR][OpenMP] Added OMPIRBuilder support for use_device_ptr clause

[Akash Banerjee via Phabricator via llvm-commits]

TIFitis added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4070-4071
+
+  for (auto &UseDeviceOp : UseDeviceInfos)
+    UseDeviceOp.second.AllocaPtr = Builder.CreateAlloca(Builder.getPtrTy());
+
----------------
`use_dev_ptr` operands must always be pointer types so I think it is safe to always just create a `ptr` for them.

However if we want to support typed pointers then we would have to create something like:
`Builder.CreateAlloca(UseDeviceOp.first->getType()->getPointerElementType())`

This is not possible as `getPointerElementType` is deprecated and I couldn't find any alternative for this.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4180-4190
+    for (auto &UseDeviceOp : UseDeviceInfos) {
+      auto *UseDeviceVal = UseDeviceOp.first;
+      auto &UseDeviceInfo = UseDeviceOp.second;
+      SmallDenseSet<Use *> ReplaceUses;
+      for (auto &Use : UseDeviceVal->uses()) {
+        if (UseDeviceInfo.Uses.find(&Use) == UseDeviceInfo.Uses.end())
+          ReplaceUses.insert(&Use);
----------------
I have commented out some code in OpenMPToLLVMIRTranslation.cpp::1489. I think that is a better way of generating code for the use_dev_ptr, but the mapValue doesn't allow remapping.

This I am stuck with letting it use the old value inside the region and replacing it later here.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:5048-5050
   Builder.restoreIP(OMPBuilder.createTargetData(
       Loc, AllocaIP, Builder.saveIP(), /* IsBegin= */ false, DeviceID,
+      /* IfCond= */ nullptr, MapInfos, UseDeviceInfos, BodyCB));
----------------
This unit test fails because of opaque pointer mismatch, The unit test runs with opaque pointers disabled however the my code only runs with it enabled.

I could not find a reasonable way to support typed pointers, I have explained this in another comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146648