[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:205
+  bool FuncModified = false;
+  for (MachineBasicBlock &MBB : MF)
+    // Cleanup missed Saddr opportunites from ISel.
----------------
Braces


================
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));
----------------
ronlieb wrote:
> arsenm wrote:
> > Braces for both half
> I thought the coding standard was no braces for single statement 'if' and 'else' , is it not ? 
> I personally like braces, however staying with convention seems best.
> I could also refactor it a little bit and avoid the whole topic...
> 
>     NewGlob = BuildMI(MBB, I, MI.getDebugLoc(), TII->get(NewOpcd));
>     if (HasVdst)
>       NewGlob->addOperand(MF, MI.getOperand(0));
> 
It was spread over multiple lines but I don't se that in the diff now


================
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
----------------
ronlieb wrote:
> arsenm wrote:
> > arsenm wrote:
> > > 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
> > I also realized the case I was worried about here probably can't happen
> seems like I should remove this test now. Originally I added it based on a concern you expressed.
> Matt, do you have a preference regarding this test?
It would still be good to have to make sure this actually works, but you need to completely replace the test body to be simpler and avoid these issues. Also I wouldn't split these tests up arbitrarily like this and merge them into one


https://reviews.llvm.org/D52846