Page MenuHomePhabricator

[AMDGPU][GlobalISel] Fold a chain of two shift instructions with constant operands
ClosedPublic

Authored by mbrkusanin on Tue, Oct 27, 3:59 AM.

Details

Summary

Sequence of same shift instructions with constant operands can be combined into
a single shift instruction.

Diff Detail

Event Timeline

mbrkusanin created this revision.Tue, Oct 27, 3:59 AM
mbrkusanin requested review of this revision.Tue, Oct 27, 3:59 AM
mbrkusanin retitled this revision from [GlobalISel] Fold a chain of two shift instructions with constant operands to [AMDGPU][GlobalISel] Fold a chain of two shift instructions with constant operands.Tue, Oct 27, 4:14 AM
foad added inline comments.Tue, Oct 27, 4:49 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1551–1553

Change to assert().

1562

I don't think Shl2Def can ever be null here, can it?

1588

"Imm" instead of "MatchInfo.Imm"

1588

Logically you want the size of the first operand here, not the size of the immediate. I'm not sure if they are guaranteed to be the same.

1595

Typo "than".

mbrkusanin marked 5 inline comments as done.
  • Addressed review comments.
  • Updated comment at the start of match function.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1562

I guess if the MIR is valid it should never be null. This match is the very similar to the one above (matchPtrAddImmedChain).

foad accepted this revision.Tue, Oct 27, 6:20 AM

LGTM, but I'm adding some generic GlobalISel reviewers in case they have comments.

This revision is now accepted and ready to land.Tue, Oct 27, 6:20 AM
arsenm added inline comments.Tue, Oct 27, 7:00 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1590

This is undefined, not 0. I don't think this needs to specially consider this case

arsenm added inline comments.Tue, Oct 27, 7:02 AM
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
57

Can't this just reuse RegisterImmPair?

llvm/test/CodeGen/AMDGPU/GlobalISel/combine-shift-imm-chain.ll
5

These cases are all SGPRs, and don't have any illegal type tests. A few MIR cases wouldn't hurt either

foad added inline comments.Tue, Oct 27, 7:06 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1590

No, this is handling cases like X >> 20 >> 25 where the sum (20 + 25) exceeds the width of the type.

foad added inline comments.Wed, Oct 28, 1:45 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1596–1597

You could also handle G_USHLSAT and G_SSHLSAT the same way as G_ASHR.

  • Using RegisterImmPair, removed ShiftChain struct.
  • Supported G_SSHLSAT and G_USHLSAT.
  • Added .mir tests
arsenm accepted this revision.Fri, Oct 30, 10:05 AM
foad requested changes to this revision.Mon, Nov 2, 7:09 AM
foad added inline comments.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1592–1594

This is wrong. With 8-bit ints:
1 sshlsat 6 = 0x40
1 sshlsat 7 = 0x7f
I think this should clamp to ScalarSizeInBits - 1 just like ashr does.

This revision now requires changes to proceed.Mon, Nov 2, 7:09 AM
foad added inline comments.Mon, Nov 2, 7:30 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1596–1597

Sorry, I was wrong about G_USHLSAT. You can't handle it quite the same way as G_ASHR. With 8-bit ints:
1 ushlsat 5 ushlsat 5 = 0xff
1 ushlsat 7 = 0x80
So they are not the same.

If the combined shift amount for ushlsat is >= 8 then you could replace it with "x == 0 ? 0 : 0xff" but it's probably not worth doing this.

arsenm added inline comments.Mon, Nov 2, 2:56 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1587

Shouldn't construct a new MachineIRBuilder here

mbrkusanin updated this revision to Diff 302525.Tue, Nov 3, 2:51 AM
  • Fix handling of G_SSHLSAT and G_USHLSAT.
  • Removed new MachineIRBuilder.
arsenm accepted this revision.Tue, Nov 3, 7:25 AM
foad added inline comments.Tue, Nov 3, 7:49 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1599

I think you mean "saturating signed or unsigned left shift" here.

But this does not work for saturating unsigned left shift. There is no good replacement for "x ushlsat 20 ushlsat 25" where 20 + 25 exceeds the width of the type. I suggest catching this case in the match func and returning false.

mbrkusanin updated this revision to Diff 303457.Fri, Nov 6, 8:50 AM
  • Avoid combine for G_USHLSAT when sum exceeds scalar size.
foad accepted this revision.Mon, Nov 9, 2:10 AM

LGTM modulo minor comment inline.

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1577–1578

The comment could be improved. (What you wrote is also true of all the other shifts.) How about "there is no simple replacement for a saturating unsigned left shift that exceeds the scalar size"?

This revision is now accepted and ready to land.Mon, Nov 9, 2:10 AM
  • Rebase
  • Updated comments