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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

This still conflates different tasks and should be separated. The definitions of the new `issue` and `wait` function need to be provided with the splitting code. Just splitting, not all the other stuff. We can then also test that.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:339-340
+      cast<Instruction>(RuntimeCall->getArgOperand(BasePtrsArgNum));
+  if (!Issue.count(BasePtrsGEP))
+    Issue.insert(BasePtrsGEP);
 
----------------
Also below, no need to check first.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:526
+  RuntimeCall->mutateFunctionType(IssueDecl.getFunctionType());
+  RuntimeCall->setCalledFunction(IssueDecl);
+  Issue.insert(RuntimeCall);
----------------
Don't mutate, I have never seen this actually. Create a new call and remove the old one.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:532
+  const unsigned WaitNumParams = 2;
+  Type ** WaitDeclParams = new Type*[WaitNumParams];
+  WaitDeclParams[0] = Type::getInt64Ty(M->getContext());
----------------
You don't need a dynamic array here and below. 


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:565-567
+  for (auto *ST : M->getIdentifiedStructTypes())
+    if (ST->getName() == "struct.tgt_async_info")
+      return ST;
----------------



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:578
+    After = I;
+  }
+}
----------------
If you define `Before = After->getNextNode();` you can `insertBefore(Before)` without resetting the insertion point all the time. Isn't `Issues` unordered?


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

https://reviews.llvm.org/D85535