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

[Justin Lebar via Phabricator via llvm-commits]

jlebar added a comment.

This is going to be so great.  Thanks, Art.



================
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);
----------------
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?


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


================
Comment at: llvm/lib/Target/NVPTX/NVPTXLowerArgs.cpp:189
+    if (isa<GetElementPtrInst>(I))
+      return llvm::all_of(I->users(), isALoadChain);
+    return isa<LoadInst>(I);
----------------
same -- is recursion ok here?


================
Comment at: llvm/lib/Target/NVPTX/NVPTXLowerArgs.cpp:205
+  if (llvm::all_of(Arg->users(), isALoadChain)) {
+    // Replace all loads with the loads in param AS which allows loading the Arg
+    // directly from parameter AS, without making a temporary copy.
----------------
Nit: run-on sentence. s/which/. This/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98469