[PATCH] D21421: [NVPTX] Improve lowering of byval args of device functions.

[Justin Lebar via llvm-commits]

jlebar added inline comments.

================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:5080
@@ +5079,3 @@
+
+  if (AddrSpaceCastSDNode *CastN = dyn_cast<AddrSpaceCastSDNode>(N)) {
+    if (CastN->getSrcAddressSpace() == ADDRESS_SPACE_GENERIC &&
----------------
As discussed, I think it would be good to have a brief comment here along the lines of

> SelectDirectAddr() recognizes addrspacecast([0->101] MoveParam(arg_symbol)) created above and returns arg_symbol.

I don't think we even need to to explain where this comes from -- it's a sufficiently generic construct that it makes sense to me we'd want to recognize it here.

================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:5084
@@ -5084,1 +5083,3 @@
+        N.getOperand(0).getOpcode() == NVPTXISD::MoveParam)
+      return (SelectDirectAddr(N.getOperand(0).getOperand(0), Address));
   }
----------------
Unnecessary parens.

================
Comment at: lib/Target/NVPTX/NVPTXLowerKernelArgs.cpp:10
@@ -9,3 +9,3 @@
 //
 // Pointer arguments to kernel functions need to be lowered specially.
 //
----------------
This comment needs to be updated.

Actually, doing any of this work in a pass called "LowerKernelArgs" seems wrong.  Should we rename the pass?

================
Comment at: lib/Target/NVPTX/NVPTXLowerKernelArgs.cpp:105
@@ +104,3 @@
+  // Kernels and regular device functions treat byval arguments
+  // differently.
+  bool runOnKernelFunction(Function &F);
----------------
Actually it seems like they handle them exactly the same?

================
Comment at: lib/Target/NVPTX/NVPTXLowerKernelArgs.cpp:233
@@ +232,3 @@
+// See comments in NVPTXDAGToDAGISel::SelectAddrSpaceCast() for the
+// details on why we need to spill byval arguments into local memory.
+bool NVPTXLowerKernelArgs::runOnDeviceFunction(Function &F) {
----------------
This comment isn't there anymore.


https://reviews.llvm.org/D21421