[PATCH] D42078: AMDGPU: Fold inline offset for loads properly in moveToVALU on GFX9

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle accepted this revision.
nhaehnle added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:3768-3770
+      if (Add &&
+          (Add->getOpcode() == AMDGPU::V_ADD_I32_e32 ||
+           Add->getOpcode() == AMDGPU::V_ADD_U32_e64)) {
----------------
mareko wrote:
> mareko wrote:
> > nhaehnle wrote:
> > > mareko wrote:
> > > > nhaehnle wrote:
> > > > > Why not V_ADD_I32_e64 and V_ADD_U32_e32 as well? Those should work the same.
> > > > > 
> > > > > You can also remove the corresponding FIXME in the comment.
> > > > > 
> > > > > Also, to support e64/VOP3, the code must check for clamp and imod (abs/neg) bits and bail out when they're set. Clamp is genuinely supported for integer ops since gfx9, and while imods will not do anything useful, they *will* affect the instruction (they just affect the MSB as if the instruction were a floating point operation).
> > > > I think that only V_ADD_I32_e64 (for gfx9) and V_ADD_U32_e32 (for others) can occur here, because integer addition is always translated to S_ADD and lowered in moveToVALU. If that is true, mods can't occur here.
> > > > 
> > > > I'll remove the FIXME.
> > > You may be right about the other opcodes.
> > > 
> > > About the mods, I still think it's better to be defensive. Just because mods can't occur here *today* doesn't mean they won't start showing up at some point in the future if saturating integer arithmetic is exposed in the frontend.
> > I can't check the mods, because the opcodes don't have any mods defined (yet). So there is nothing to do.
> moveToVALU selects it from S_ADD.
Huh, that's odd, but you're right. Looks like integer clamping is only half-implemented after all, and not even the (dis)assembler really supports it.


Repository:
  rL LLVM

https://reviews.llvm.org/D42078