Page MenuHomePhabricator

[InstCombine] recognize bswap disguised as shufflevector
ClosedPublic

Authored by spatel on Thu, Aug 29, 10:50 AM.

Details

Summary

bitcast <N x i8> (shuf X, undef, <N, N-1,...0>) to i{N*8}--> bswap (bitcast X to i{N*8})

In PR43146:
https://bugs.llvm.org/show_bug.cgi?id=43146
...we have a more complicated case where SLP is making a mess of bswap. This patch won't do anything for that currently, but we need to improve bswap recognition in instcombine, SLP, and/or a standalone pass to avoid that problem.

This is limited using the data-layout so we don't try to do this transform with actual vector types. The backend does not appear to have folds to convert in either direction, so we don't want to mess up something that is actually better lowered as a shuffle.

On x86, we're trading something like this:

vmovd	%edi, %xmm0
vpshufb	LCPI0_0(%rip), %xmm0, %xmm0 ## xmm0 = xmm0[3,2,1,0,u,u,u,u,u,u,u,u,u,u,u,u]
vmovd	%xmm0, %eax

For:

movl	%edi, %eax
bswapl	%eax

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Thu, Aug 29, 10:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Aug 29, 10:50 AM

Looks good.
The same applies to llvm.bitreverse.*, i believe, if scalar type is i1?

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2397–2412 ↗(On Diff #217937)

Precommit please

spatel marked an inline comment as done.Thu, Aug 29, 12:45 PM

Looks good.
The same applies to llvm.bitreverse.*, i believe, if scalar type is i1?

Yes. That one is probably rarer and might want slightly different constraints. I'll add a TODO for now.

spatel updated this revision to Diff 217959.Thu, Aug 29, 12:47 PM

Patch updated:

  1. Rebased after cleanup - rL370399.
  2. Added TODO comment about bitreverse.

Some more thoughts, but i think this looks ok.

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2424–2426 ↗(On Diff #217959)

Should Shuf->isReverse() be checked last, it is likely most costly check here?

llvm/test/Transforms/InstCombine/bswap.ll
284 ↗(On Diff #217959)

I think you also want a test where the shuffle mask is widening (to a legal type)
We currently won't accept it, but we might one day, and i'm not sure what that code would do.

spatel updated this revision to Diff 217969.Thu, Aug 29, 2:02 PM
spatel marked 2 inline comments as done.

Patch updated:

  1. Optimization: rearranged comparisons for better perf.
  2. Added widening shuffle negative test.
lebedev.ri accepted this revision.Thu, Aug 29, 2:09 PM

No further comments from me here, thanks.

This revision is now accepted and ready to land.Thu, Aug 29, 2:09 PM
This revision was automatically updated to reflect the committed changes.