[PATCH] D52846: [AMDGPU] Add FixupVectorISel pass, currently Supports SREGs in GLOBAL LD/ST

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/SIFixupVectorISel.cpp:167-172
+    if (HasVdst)
+      NewGlob = BuildMI(MBB, I, MI.getDebugLoc(), TII->get(NewOpcd),
+                        MI.getOperand(0).getReg());
+    else
+      // No vdst field.
+      NewGlob = BuildMI(MBB, I, MI.getDebugLoc(), TII->get(NewOpcd));
----------------
Braces for both half


================
Comment at: test/CodeGen/AMDGPU/conv2d-saddr.ll:16
+; Function Attrs: convergent nounwind
+define hidden amdgpu_kernel void @simpleConv2d(i32 addrspace(1)* nocapture %dst_image, i32 addrspace(1)* nocapture readonly %src_image, i32 addrspace(1)* nocapture readonly %conv_kernel) local_unnamed_addr #0 !kernel_arg_addr_space !5 !kernel_arg_access_qual !6 !kernel_arg_type !7 !kernel_arg_base_type !7 !kernel_arg_type_qual !8 {
+entry:
----------------
ronlieb wrote:
> arsenm wrote:
> > This testcase is way too big. Most of the tests should be only a handful of instructions. There should be ones stressing the immediate limits. I also don't see ones for the atomics that the comments say work
> reduced test case to 47 lines total.
This is still too big. You only need not much more than load, a store, and a GEP in each function


================
Comment at: test/CodeGen/AMDGPU/global-saddr-misc.ll:7-11
+define amdgpu_cs void @_amdgpu_cs_main(<3 x i32> inreg %arg) {
+bb:
+  %tmp = extractelement <3 x i32> %arg, i32 1
+  %tmp1 = inttoptr i32 %tmp to <4 x i32> addrspace(1)*
+  %tmp2 = load <4 x i32>, <4 x i32> addrspace(1)* %tmp1, align 16
----------------
You don't need the complicated vector stuff (this is also broken since addrspace(1) is 64-bit). The pointer argument needs to be the direct argument here. It won't be since now there will end up being a zext to the pointer size, so this isn't testing what it's supposed to


https://reviews.llvm.org/D52846