Page MenuHomePhabricator

[X86] Improve i8 + 'slow' i16 funnel shift codegen
ClosedPublic

Authored by RKSimon on Sat, May 23, 3:09 AM.

Details

Summary

This is a preliminary patch before I deal with the xor+and issue raised in D77301.

We get much better code for i8/i16 funnel shifts by concatenating the operands together and performing the shift as a double width type, it avoids repeated use of the shift amount and partial registers.

fshl(x,y,z) -> (((zext(x) << bw) | zext(y)) << (z & (bw-1))) >> bw.
fshr(x,y,z) -> (((zext(x) << bw) | zext(y)) >> (z & (bw-1))) >> bw.

Alive2: http://volta.cs.utah.edu:8080/z/CZx7Cn

This doesn't do as well for i32 cases on x86_64 (the xor+and followup patch is much better) so I haven't bothered with that.

Cases with constant amounts are more dubious as well so I haven't currently bothered with those - its these kind of 'edge' cases that put me off trying to put this in TargetLowering::expandFunnelShift.

Diff Detail

Event Timeline

RKSimon created this revision.Sat, May 23, 3:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptSat, May 23, 3:09 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
lebedev.ri added inline comments.Sat, May 23, 4:10 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
19094

This can be anyext

RKSimon updated this revision to Diff 265858.Sat, May 23, 7:04 AM

Use anyextend and always extend to i32 straight away (as I said i32 funnel shifts as i64 didn't make much sense so I've dropped that generalization).

LGTM

For fshl case, we could introduce some more ILP: http://volta.cs.utah.edu:8080/z/UJ6viM
https://godbolt.org/z/xsJgPb https://godbolt.org/z/5W26NV
Not sure it would be an improvement?
As a sidenote, we clearly don't fold to either variant in DAGCombiner.

LGTM

For fshl case, we could introduce some more ILP: http://volta.cs.utah.edu:8080/z/UJ6viM
https://godbolt.org/z/xsJgPb https://godbolt.org/z/5W26NV
Not sure it would be an improvement?
As a sidenote, we clearly don't fold to either variant in DAGCombiner.

Looking at these cases in llvm-mca with 'slow shld' targets (btver2/bdver2/znver*) the naive cases all seem to give better throughput

lebedev.ri accepted this revision.Sat, May 23, 11:50 AM
This revision is now accepted and ready to land.Sat, May 23, 11:50 AM
This revision was automatically updated to reflect the committed changes.