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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

> 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.



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


================
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);
----------------
Why is the load required here and not for devPtr?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146648