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

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

hamax97 added a comment.

I think if I only add the declarations of `__tgt_target_data_begin_issue` and `__tgt_target_data_being_wait` the linking process will fail. How should I define the `wait` function?, the `issue` is just a wrapper of the current function, but as we don't have Shilei's patch merged yet I would have to create something that does nothing and change it later when the asynchronous stuff is merged.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:339-340
+      cast<Instruction>(RuntimeCall->getArgOperand(BasePtrsArgNum));
+  if (!Issue.count(BasePtrsGEP))
+    Issue.insert(BasePtrsGEP);
 
----------------
jdoerfert wrote:
> Also below, no need to check first.
Sure, I'll change it, I forgot `SetVector` ignore repeated elements.


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


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:532
+  const unsigned WaitNumParams = 2;
+  Type ** WaitDeclParams = new Type*[WaitNumParams];
+  WaitDeclParams[0] = Type::getInt64Ty(M->getContext());
----------------
jdoerfert wrote:
> You don't need a dynamic array here and below. 
I didn't know what to do. I think we need a long life array because the array will be erased when the function scope ends, won't it?.


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


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:578
+    After = I;
+  }
+}
----------------
jdoerfert wrote:
> If you define `Before = After->getNextNode();` you can `insertBefore(Before)` without resetting the insertion point all the time. Isn't `Issues` unordered?
Ohh sure, that is handier. But `Issue` is not unordered, if we mess up the order of the instructions we change the semantics. What is nice about `SetVector` is that it allows you to visit its elements in the order they were inserted.


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

https://reviews.llvm.org/D85535