This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Use gfx9 carry-less add/sub instructions
ClosedPublic

Authored by arsenm on Nov 16 2017, 4:30 PM.

Details

Reviewers
rampitec
Summary

A few more places still need to be updated, but this is most of them.

Diff Detail

Event Timeline

arsenm created this revision.Nov 16 2017, 4:30 PM
rampitec added inline comments.Nov 16 2017, 4:51 PM
lib/Target/AMDGPU/SIInstrInfo.cpp
2846โ€“2847

Should not we always return V_ADD_I32_e32 here?

2850

Same here.

3893

It needs assert(Inst.getOpcode() == AMDGPU::S_ADD_I32 || Inst.getOpcode() == AMDGPU::S_SUB_I32)

arsenm added inline comments.Nov 17 2017, 12:19 PM
lib/Target/AMDGPU/SIInstrInfo.cpp
2846โ€“2847

One of these is dead. We always select S_ADD_I32/S_SUB_I32. We don't select these with uses of the SCC value, so I don't think it matters we use.

I tried swapping this to always select s_add_u32 for add, but that has a similar problem later. We select s_addk_i32 from s_add_i32, and not s_add_u32. That's formed a lot later, where it's more questionable to make assumptions about how SCC is getting used. We're missing optimizations to try to use SCC conditions, but ideally there would be some.

rampitec accepted this revision.Nov 17 2017, 12:21 PM

LGTM with the assertion added to moveScalarAddSub.

This revision is now accepted and ready to land.Nov 17 2017, 12:21 PM
arsenm updated this revision to Diff 123404.Nov 17 2017, 12:55 PM

Add assert, more switch cases

rampitec requested changes to this revision.Nov 17 2017, 1:02 PM
rampitec added inline comments.
lib/Target/AMDGPU/SIInstrInfo.cpp
3897

Now you can send here S_ADD_U32 and you do not want to convert it into V_SUB_U32_e64.

This revision now requires changes to proceed.Nov 17 2017, 1:02 PM
arsenm added inline comments.Nov 17 2017, 2:22 PM
lib/Target/AMDGPU/SIInstrInfo.cpp
3897

S_ADD_U32 is never selected, so it never reaches here. I added it to the other switch so that in case it is selected, it will hit the assert here.

rampitec accepted this revision.Nov 17 2017, 2:25 PM
This revision is now accepted and ready to land.Nov 17 2017, 2:25 PM
arsenm updated this revision to Diff 124841.Nov 29 2017, 3:50 PM

Skip s_add_u32. It's still used in one case where the carry is used. Cleaning this up will be a more involved separate step

arsenm closed this revision.Nov 30 2017, 2:52 PM

r319491