[PATCH] D85535: [OpenMPOpt][SplitMemTransfer][WIP] Splitting the runtime call __tgt_target_data_begin

[Hamilton Tobon-Mosquera via Phabricator via llvm-commits]

hamax97 added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:48
+    cl::desc("Tries to hide the latency of host to device memory transfers"),
+    cl::Hidden, cl::init(false));
+
----------------
jdoerfert wrote:
> Call the variable and option "hide memory transfer latency" (or similar) and add a WIP to the description.
Sure


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:692
+      Changed |= split(RTCall);
+      return Changed;
+    };
----------------
jdoerfert wrote:
> This is not the correct return value. The lambda needs to return if the use was deleted or not. So you need to track "globally changed" (which is `Changed` right now), and "use changed" (which you return from the lambda).
Sure, I'll fix it. If I understood I need to return from the lambda the immediate value of `split`, without forgetting to make an or with the outer `Changed`.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:700
+  /// Splits \p RuntimeCall into its "issue" and "wait" counterparts.
+  bool split(CallInst *RuntimeCall) {
+    auto &IRBuilder = OMPInfoCache.OMPBuilder;
----------------
jdoerfert wrote:
> We need a more descriptive name here, e.g., `splitTargetDataBeginRTC`.
Sure.


================
Comment at: llvm/test/Transforms/OpenMP/hide_mem_transfer_latency.ll:2
-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: -p --function-signature
-; RUN: opt -S -passes=openmpopt < %s | FileCheck %s
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
----------------
jdoerfert wrote:
> Can you make an NFC pre-commit that updates the test, and then only include the `-openmp-split-memtransfers` changes in this patch.
Sure, but the test will fail, we would have to commit them at the same time.


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

https://reviews.llvm.org/D85535