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

[Ron Lieberman via Phabricator via llvm-commits]

ronlieb added inline comments.


================
Comment at: lib/Target/AMDGPU/SIFixupVectorISel.cpp:190
+    MI.eraseFromParent();
+    LLVM_DEBUG(dbgs() << "New Global Mem " << *NewGlob << '\n');
+  }
----------------
arsenm wrote:
> This is probably just noise for general debugging 
It is indeed for general debugging.
Is there a specific request implied by your comment?
would you prefer this be removed?


================
Comment at: lib/Target/AMDGPU/SIFixupVectorISel.cpp:205
+  bool FuncModified = false;
+  for (MachineBasicBlock &MBB : MF)
+    // Cleanup missed Saddr opportunites from ISel.
----------------
arsenm wrote:
> Braces
This is at the moment a single statement loop, I would expect it to become compound at some point at which braces would be inserted. 
See example: https://github.com/llvm-mirror/llvm/blob/master/lib/CodeGen/BranchRelaxation.cpp line 163


================
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
----------------
arsenm wrote:
> 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
I think part of your your suggestion is to reduce the current number of new tests down to 1 or 2.

I would propose changing these   
 test/CodeGen/AMDGPU/conv2d-saddr.ll (47 lines)   
 test/CodeGen/AMDGPU/global-load_stores.mir (86 lines)  
 test/CodeGen/AMDGPU/global-saddr-atomics.ll (359 lines)  
 test/CodeGen/AMDGPU/global-saddr-misc.ll (13 lines)  
 test/CodeGen/AMDGPU/global-saddr-offsets.ll (60 lines) 
to 

1) fold these 
 test/CodeGen/AMDGPU/global-load_stores.mir (86 lines)  
 test/CodeGen/AMDGPU/global-saddr-atomics.ll (359 lines)  
into a single .mir coverage test
 test/CodeGen/AMDGPU/global-load-stores-atomics.mir (86 lines)  

2) fold these 
 test/CodeGen/AMDGPU/conv2d-saddr.ll (47 lines)   
 test/CodeGen/AMDGPU/global-saddr-misc.ll (13 lines)  
 test/CodeGen/AMDGPU/global-saddr-offsets.ll (60 lines) 
into a single test
 test/CodeGen/AMDGPU/global-saddr.ll





https://reviews.llvm.org/D52846