Page MenuHomePhabricator

AMDGPU: Fold inline offset for loads properly in moveToVALU on GFX9
ClosedPublic

Authored by mareko on Jan 15 2018, 9:41 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

mareko created this revision.Jan 15 2018, 9:41 AM
nhaehnle added inline comments.Jan 24 2018, 9:14 AM
lib/Target/AMDGPU/SIInstrInfo.cpp
3768–3770 ↗(On Diff #129876)

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).

mareko added inline comments.Jan 25 2018, 4:32 PM
lib/Target/AMDGPU/SIInstrInfo.cpp
3768–3770 ↗(On Diff #129876)

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.

nhaehnle added inline comments.Jan 26 2018, 7:56 AM
lib/Target/AMDGPU/SIInstrInfo.cpp
3768–3770 ↗(On Diff #129876)

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.

mareko added inline comments.Jan 29 2018, 5:11 PM
lib/Target/AMDGPU/SIInstrInfo.cpp
3768–3770 ↗(On Diff #129876)

I can't check the mods, because the opcodes don't have any mods defined (yet). So there is nothing to do.

arsenm added inline comments.Jan 29 2018, 5:15 PM
lib/Target/AMDGPU/SIInstrInfo.cpp
3769 ↗(On Diff #129876)

It's a defect that at this point the e32 version is ever encountered. We shouldn't be emitting VCC uses up front. Where is this coming from? I thought I fixed most of these places already

mareko added inline comments.Jan 29 2018, 6:46 PM
lib/Target/AMDGPU/SIInstrInfo.cpp
3768–3770 ↗(On Diff #129876)

moveToVALU selects it from S_ADD.

3769 ↗(On Diff #129876)

moveToVALU selects it from S_ADD.

nhaehnle accepted this revision.Jan 30 2018, 3:18 AM
nhaehnle added inline comments.
lib/Target/AMDGPU/SIInstrInfo.cpp
3768–3770 ↗(On Diff #129876)

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.

This revision is now accepted and ready to land.Jan 30 2018, 3:18 AM
This revision was automatically updated to reflect the committed changes.