This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Fold or of shifts to funnel shifts.
ClosedPublic

Authored by abinavpp on Nov 23 2021, 9:04 PM.

Details

Summary

This change folds a basic funnel shift idiom:

  • (or (shl x, amt), (lshr y, sub(bw, amt))) -> fshl(x, y, amt)
  • (or (shl x, sub(bw, amt)), (lshr y, amt)) -> fshr(x, y, amt)

This also helps in folding to rotate shift if x and y are equal since we
already have a funnel shift to rotate combine.

Diff Detail

Event Timeline

abinavpp created this revision.Nov 23 2021, 9:04 PM
abinavpp requested review of this revision.Nov 23 2021, 9:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2021, 9:04 PM
foad added a comment.Nov 24 2021, 7:31 AM

(or (shl x, amt), (lshr y, sub(bw, amt)) -> fshl(x, y)
(or (shl x, sub(bw, amt), (lshr y, amt)) -> fshr(x, y)

Funnel shifts take three arguments "x, y, amt".

foad added inline comments.Nov 24 2021, 7:39 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3881

Missing right paren after sub(bw, amt).

3889

Missing right paren after sub(bw, amt).

llvm/test/CodeGen/AMDGPU/GlobalISel/combine-fsh.mir
18

Huh? The funnel shift amount should surely be %amt not %bw.

abinavpp added inline comments.Nov 24 2021, 6:24 PM
llvm/test/CodeGen/AMDGPU/GlobalISel/combine-fsh.mir
18

D'oh!

abinavpp updated this revision to Diff 389636.Nov 24 2021, 6:36 PM

Addressed review comments.

abinavpp edited the summary of this revision. (Show Details)Nov 24 2021, 6:41 PM
abinavpp marked 3 inline comments as done.
foad accepted this revision.Nov 25 2021, 1:15 AM

LGTM. But why doesn't it already work on vectors? And would it be worth adding support for constant shift amounts like (or (shl x, 20), (lshr x, 12))?

This revision is now accepted and ready to land.Nov 25 2021, 1:15 AM

But why doesn't it already work on vectors?

We're not matching the constant splat here. D114625 should fix this, if I haven't missed anything.

And would it be worth adding support for constant shift amounts like (or (shl x, 20), (lshr x, 12))?

Yes, we need to extend the pattern match.

This revision was automatically updated to reflect the committed changes.

And would it be worth adding support for constant shift amounts like (or (shl x, 20), (lshr x, 12))?

Yes, we need to extend the pattern match.

D116529