[PATCH] D120129: [NVPTX] Enhance vectorization of ld.param & st.param

[Artem Belevich via Phabricator via llvm-commits]

tra added a subscriber: yaxunl.
tra added a comment.

In D120129#3390733 <https://reviews.llvm.org/D120129#3390733>, @kovdan01 wrote:

> @tra Thanks for your comments! Updated the patch according the discussion about forcing alignment 16.
>
>> I think we should be able to do that to all no-kernel functions if we're compiling without -fgpu-rdc. I think we do reduce visibility of non-kernels in that case, but it would be good to make sure.
>
> Checked if we do reduce visibility in such cases, and looks like we do not.

I was indeed mistaken. AFAICT, we only internalize some device-side variables.
@yaxunl - Sam, do we change visibility for anything else? I know we must keep kernels visible, but the question is whether we ever internalize non-kernel functions and if not, whether we want to. In this case it would allow us to bump argument and return value alignment.

> Also, searching for `GPURelocatableDeviceCode` through LLVM codebase does not get results where this value is checked in context of reducing function visibility. I could implement that change, and IMHO that should be a separate patch.

Yeah, it's a separate issue.

> Regarding the current patch – how is https://reviews.llvm.org/D118084 going? Can we merge this patch without waiting for your change about passing byval aggregates directly? As I already mentioned in my previous comment, I suppose the change is useful as far as compiler should work good with any type of IR maybe even generated by non-clang frontend.

There's been little progress on that patch lately. I've got way less time to work on all related changes than I hoped for. This patch does not depend on my changes and will not be blocked. I was mostly pointing at it as a heads up that if/when it lands we would no longer see byval pointers in clang-generated IR and the impact of this patch would be reduced.



================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:4319-4320
+/// getFunctionParamOptimizedAlign - since function arguments are passed via
+/// .param space, we may want to have increased alignmet in it to enhance
+/// vectorization options. We can increase alignment only if the function has
+/// internal or private linkage as for other linkage types callers may already
----------------
"..may want to increase their alignment in a way that ensures that we can effectively vectorize their loads & stores."



================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:4321
+/// vectorization options. We can increase alignment only if the function has
+/// internal or private linkage as for other linkage types callers may already
+/// rely on default linkage. This function returns maximum from alignment 16
----------------
"or has private linkage"


================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:4322
+/// internal or private linkage as for other linkage types callers may already
+/// rely on default linkage. This function returns maximum from alignment 16
+/// and default alignment. Alignment 16 is chosen because it's maximum size
----------------
Did you mean "rely on default alignment" instead of "linkage"?

"To allow using 128-bit vectorized loads/stores. his function ensures that alignment is 16 or greater."


================
Comment at: llvm/lib/Target/NVPTX/NVPTXLowerArgs.cpp:235
+// NVPTXTargetLowering::LowerCall.
+static void tryIncreaseByValAlign(Argument *Arg, Value *ArgInParamAS,
+                                  const NVPTXTargetLowering *TLI) {
----------------
"tryIncreaseByValAlign" does not sound quite right.  
"adjustByValArgAlignment()" ?

I'd also mention that we traverse all loads from byval pointer and adjust their alignment, if those were using known offset.



================
Comment at: llvm/test/CodeGen/NVPTX/param-vectorize.ll:38
+;     DECLARE_CALLEE(StName)      \
+;     DECLARE_CALLER(StName)      \
+; 
----------------
We should also check that alignment of kernel parameters is not affected.


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

https://reviews.llvm.org/D120129