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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert 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));
+
----------------
Call the variable and option "hide memory transfer latency" (or similar) and add a WIP to the description.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:692
+      Changed |= split(RTCall);
+      return Changed;
+    };
----------------
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).


================
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;
----------------
We need a more descriptive name here, e.g., `splitTargetDataBeginRTC`.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:710
+    SmallVector<Value *, 7> Args;
+    Args.reserve(RuntimeCall->getNumArgOperands());
+    for (auto &Arg : RuntimeCall->args())
----------------
Let's do 8 not 7, no need for the reserve.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:715
+    CallInst *IssueCallsite = CallInst::Create(
+        IssueDecl, ArrayRef<Value *>(Args), "handle", RuntimeCall);
+    RuntimeCall->removeFromParent();
----------------
Nit: No need for the ArrayRef.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:716-717
+        IssueDecl, ArrayRef<Value *>(Args), "handle", RuntimeCall);
+    RuntimeCall->removeFromParent();
+    RuntimeCall->deleteValue();
+
----------------



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:724-731
+    // Add "wait" call site.
+    const unsigned WaitNumParams = 2;
+    Value *WaitParams[] = {
+        IssueCallsite->getArgOperand(0), // device_id.
+        IssueCallsite // returned handle.
+    };
+    CallInst::Create(
----------------



================
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"
----------------
Can you make an NFC pre-commit that updates the test, and then only include the `-openmp-split-memtransfers` changes in this patch.


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

https://reviews.llvm.org/D85535