[PATCH] D98469: [NVPTX] Avoid temp copy of byval kernel parameters.

[Artem Belevich via Phabricator via llvm-commits]

tra added inline comments.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXLowerArgs.cpp:165-166
+  if (auto *LI = dyn_cast<LoadInst>(I)) {
+    auto *NewLI = new LoadInst(LI->getType(), Param, LI->getName(),
+                               LI->isVolatile(), LI->getAlign(), I);
+    LI->replaceAllUsesWith(NewLI);
----------------
jlebar wrote:
> Looking at https://llvm.org/docs/LangRef.html#load-instruction, there's a lot of additional metadata that might be on a load instruction.  Do we need to copy it?  (Is there no LLVM utility that says, "create a new load in a different address space?)
> 
> Also, should we disallow atomic loads or otherwise handle them?
Good point. I do not even need to clone it -- it does not care about address space itself. Its pointer operand deals with it.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXLowerArgs.cpp:179
+    llvm::for_each(UsersToUpdate,
+                   [NewGEP](Value *V) { ConvertToParamAS(V, NewGEP); });
+    GEP->eraseFromParent();
----------------
jlebar wrote:
> Is it safe to write this recursively?  (Stack overflow?)
It could be a problem.

LLVM's manual does not mention anything about that, but the examples are all 'flat'. 
https://llvm.org/docs/ProgrammersManual.html#basic-inspection-and-traversal-routines

Shouldn't be hard to rewrite it as a loop. 

@asbirlea : is recursion OK here, or do I need to use a loop instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98469