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

[Artem Belevich via llvm-commits]

tra added inline comments.

================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:692
@@ +691,3 @@
+        // use address of local copy if it's needed.
+        //
+        // In order for this bit to do its job we need to:
----------------
jlebar wrote:
> Based on this comment, I am unsure why this transformation is correct:
> 
> Suppose someone takes the address of a local variable and then writes to that address.  That write needs to show up for everyone who reads from the param.  Which means that everyone needs to be using that local copy.  Which really means that the transformation we're describing is only safe because of how we lower the kernel args in the IR transformation.
> 
> So I think we should say something about that, if that makes any sense.
In current code because we move value of param pointer to a register, it magically gets spilled to local memory by ptxas and we actually get a pointer to that local copy. If we want to use that pointer to modify something, we'll be modifying local copy. However, if we attempt to read from param space as well, that will be a problem. You can probably do that with inline asm, but it will not happen with IR we generate -- neither currently, nor after the patch. In both cases IR can only do writes to a local copy (the only way to access parameter in IR is via that pointer to a local copy). With this patch direct reads from param space will only happen if we don't modify local copy, so we're safe there, too.


================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:704
@@ +703,3 @@
+        // pointer to local copy of the arg. All this is done in
+        // NVPTXLowerKernelArgs.
+
----------------
jlebar wrote:
> OK, this is the argument I was looking for above.  Maybe we just need a pointer there down to this paragraph ("This is safe because of how we lower kernel args in IR, see (a) below." or something.)
I've added a reference there to step (a).

================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:710
@@ +709,3 @@
+        // LowerFormalArguments(). Now it just lowers argument to
+        // NVPTXISD::MoveParam without any space conversion.
+
----------------
jlebar wrote:
> This is a bit confusing as a comment, because it's describing a change relative to code that no longer exists.  I'm not sure this part is necessary at all, actually, which is maybe good because long comment is long.  :)
Simplified to just describe where/how and arg is lowered to MoveParam.


http://reviews.llvm.org/D21421