[PATCH] D21421: [NVPTX] Improve lowering of byval args of device functions.
Justin Lebar via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 16 18:44:10 PDT 2016
jlebar added a comment.
This is great, thank you. I wanted to try to help with articles and commas, so I pointed out all of them; hope that's not too annoying.
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:678
@@ +677,3 @@
+ // space and is accessed via ld.local and st.local
+ // instructions.
+ //
----------------
Nit, missing close quote.
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:682
@@ +681,3 @@
+ // currently broken for byval arguments with alignment < 4 for
+ // sm_50+ in all public versions of CUDA. Taking address of
+ // such argument results in SASS code that attempts to store
----------------
the address
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:683
@@ +682,3 @@
+ // sm_50+ in all public versions of CUDA. Taking address of
+ // such argument results in SASS code that attempts to store
+ // the value using 32-bit write to an unaligned address.
----------------
an argument
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:684
@@ +683,3 @@
+ // such argument results in SASS code that attempts to store
+ // the value using 32-bit write to an unaligned address.
+ //
----------------
a 32-bit write
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:686
@@ +685,3 @@
+ //
+ // Optimization: On top of the overhead of spill-to-stack we
+ // also convert that address from local to generic space which
----------------
spill-to-stack,
(Comma after long prepositional phrase at the beginning of a sentence.)
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:687
@@ +686,3 @@
+ // Optimization: On top of the overhead of spill-to-stack we
+ // also convert that address from local to generic space which
+ // may result in further overhead. All of it is in most cases
----------------
space, which
(Comma before non-exclusive "which" clause. It's the difference between
"I use the compiler which I prefer." (I choose my favorite compiler from among a variety of options.)
"I use the compiler, which I prefer." (I use a compiler, as opposed to an assembler, or writing bits out with a magnetized needle.))
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:689
@@ +688,3 @@
+ // may result in further overhead. All of it is in most cases
+ // unnecessary as we only need the value of the variable and
+ // it can be accessed directly by using argument symbol. We'll
----------------
unnecessary, as
variable, and
(Comma before conjunction.)
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:690
@@ +689,3 @@
+ // unnecessary as we only need the value of the variable and
+ // it can be accessed directly by using argument symbol. We'll
+ // use address of local copy if it's needed.
----------------
the argument
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:691
@@ +690,3 @@
+ // it can be accessed directly by using argument symbol. We'll
+ // use address of local copy if it's needed.
+ //
----------------
the address of the local copy
================
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:
----------------
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.
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:694
@@ +693,3 @@
+ // In order for this bit to do its job we need to:
+
+ // a) Let LLVM do the spilling and make sure IR does not
----------------
Would suggest keeping the "//"s here, so it doesn't look like separate comments (and is consistent with above).
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:695
@@ +694,3 @@
+
+ // a) Let LLVM do the spilling and make sure IR does not
+ // access byval arguments directly. Instead argument pointer
----------------
sure the IR
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:696
@@ +695,3 @@
+ // a) Let LLVM do the spilling and make sure IR does not
+ // access byval arguments directly. Instead argument pointer
+ // (which is in default address space) is addrspacecast'ed to
----------------
Instead, the argument pointer
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:697
@@ +696,3 @@
+ // access byval arguments directly. Instead argument pointer
+ // (which is in default address space) is addrspacecast'ed to
+ // param space and the data it points to is copied to
----------------
the default address space
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:698
@@ +697,3 @@
+ // (which is in default address space) is addrspacecast'ed to
+ // param space and the data it points to is copied to
+ // allocated local space (i.e. we let compiler spill it which
----------------
, and the data
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:699
@@ +698,3 @@
+ // param space and the data it points to is copied to
+ // allocated local space (i.e. we let compiler spill it which
+ // gives us control over when it happens and an opportunity to
----------------
it, which
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:700
@@ +699,3 @@
+ // allocated local space (i.e. we let compiler spill it which
+ // gives us control over when it happens and an opportunity to
+ // optimize the spill away later if nobody needs its
----------------
s/it/the spill/
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:700
@@ +699,3 @@
+ // allocated local space (i.e. we let compiler spill it which
+ // gives us control over when it happens and an opportunity to
+ // optimize the spill away later if nobody needs its
----------------
jlebar wrote:
> s/it/the spill/
Suggest "and gives us an opportunity" -- we're now a bit far from the original "gives".
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:704
@@ +703,3 @@
+ // pointer to local copy of the arg. All this is done in
+ // NVPTXLowerKernelArgs.
+
----------------
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.)
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:710
@@ +709,3 @@
+ // LowerFormalArguments(). Now it just lowers argument to
+ // NVPTXISD::MoveParam without any space conversion.
+
----------------
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. :)
================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:715
@@ +714,3 @@
+ // from step(b) with arg symbol itself which can then be used
+ // for [symbol+offset] addressing.
+
----------------
The second sentence is a bit confusing, maybe we can say
It replaces the addrspacecast (MoveParam) from step (a) with the arg symbol itself. This can then be used for [symbol + offset] addressing.
http://reviews.llvm.org/D21421
More information about the llvm-commits
mailing list