[PATCH] D59829: AMDGPU: An extension to promote constant offset to the immediate

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:1230-1231
+  if (!BaseLoDef ||  !BaseHiDef ||
+      (BaseLoDef->getOpcode() != AMDGPU::V_ADD_I32_e32 &&
+       BaseLoDef->getOpcode() != AMDGPU::V_ADD_I32_e64)   ||
+      (BaseHiDef->getOpcode() != AMDGPU::V_ADDC_U32_e32 &&
----------------
cfang wrote:
> arsenm wrote:
> > cfang wrote:
> > > arsenm wrote:
> > > > Where are the _e32 versions coming from? I don't think you should be seeing these at this point.
> > > > 
> > > > You would need to verify the carry out is dead here. You should add a testcase where the vcc def of the add is used
> > > These _e32 version was generated in "SI Fold Operands". If you think should should not happen, we may need to fix that first.
> > This one is complicated. This still needs verification that the vcc use isn't needed
> Do you mean the add AMDGPU::V_ADD_I32_e32 will actually been removed?
> 
> >This still needs verification that the vcc use isn't needed
> 
> How to do this verification?
.isDead() may be sufficient?


================
Comment at: test/CodeGen/AMDGPU/promote-constOffset-to-imm.ll:493
+bb:
+  %tmp = tail call i64 @_Z13get_global_idj(i32 0)
+  %tmp1 = and i64 %tmp, 255
----------------
cfang wrote:
> arsenm wrote:
> > This shouldn't include a call
> Do you mean in general a LIT test shouldn't include a call, or just this test case? I saw a lot in the existing lit tests.
Lit tests should generally not involve calls unless the call is specifically being tested. They add a lot of code and test complexity, and since the ABI is going to change, they may not be stable.


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

https://reviews.llvm.org/D59829