[PATCH] D146648: [MLIR][OpenMP] Added MLIR translation support for use_device clauses

Thu Aug 3 03:31:49 PDT 2023

TIFitis marked 2 inline comments as done.
TIFitis added a comment.

In D146648#4554063 <https://reviews.llvm.org/D146648#4554063>, @kiranchandramohan wrote:

>> use_device_ptr: The mapped variables marked with use_device_ptr are accessed through new pointer allocas inside the region.
>> use_device_addr: The mapped variables marked with use_device_addr are accessed through the base pointer mappers.
>
> This explanation in the summary talks of mapped variables. Mapping is always not necessary. Could you also expand the explanation a little more? And also talk a bit about the difference between use_device_ptr and use_device_addr.

The regular behaviour is for mapping to be present. In the event that mapping information is absent we simply don't have the map_type and size information available. As such according to the specification the size gets set to 0 and the map_type bits are omitted, i.e, default alloc/release is used as appropriate.

================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1560
+      // Check if any device ptr/addr info is available
+      if (!info.DevicePtrInfoMap.empty()) {
+        builder.restoreIP(codeGenIP);
----------------
kiranchandramohan wrote:
> If you do not need any codegen in this case for other switch options as well, then you can consider moving the `if` outside the switch.
No, we need the if condition here in combination with the switch case to generate code for device privatisation.

================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1574-1575
+          const auto &arg = region.front().getArgument(argIndex);
+          auto *LI = builder.CreateLoad(
+              builder.getPtrTy(), info.DevicePtrInfoMap[mapOpValue].second);
+          moduleTranslation.mapValue(arg, LI);
----------------
kiranchandramohan wrote:
> Why is the load required here and not for devPtr?
In case of dev_ptr the load inst is added by OMPIRBuilder itself along with a store Inst to store it into the new temp ptr var added.

For dev_addr we don't add the LoadInst in the OMPIRBuilder only because of compatibility issues with Clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146648