This is an archive of the discontinued LLVM Phabricator instance.

[X86] Add a combine for back to back VSRAI instructions
ClosedPublic

Authored by craig.topper on Nov 27 2018, 10:57 AM.

Details

Summary

Expansion of SIGN_EXTEND_INREG can create a VSRAI instruction. If there is already a VSRAI after it, we should combine them into a larger VSRAI

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Nov 27 2018, 10:57 AM
spatel added inline comments.Nov 27 2018, 11:52 AM
lib/Target/X86/X86ISelLowering.cpp
35471 ↗(On Diff #175533)

I don't think we should limit this with hasOneUse (we don't have that restriction in the equivalent generic DAG or IR folds). Add a test for that scenario?

test/Analysis/CostModel/X86/testshiftashr.ll
264 ↗(On Diff #175533)

Is it weird that we're (only partially) checking codegen in a cost model test file?

craig.topper marked 2 inline comments as done.Nov 27 2018, 12:11 PM
craig.topper added inline comments.
lib/Target/X86/X86ISelLowering.cpp
35471 ↗(On Diff #175533)

Without it we generated extra moves on some other test cases in vector-shift-ashr-sub128.ll at least on pre-AVX targets.

test/Analysis/CostModel/X86/testshiftashr.ll
264 ↗(On Diff #175533)

Yeah its pretty weird. We didn't even have complete codegen test coverage of the narrow vectors until I added the sub128 tests yesterday.

Remove one use check.

spatel accepted this revision.Nov 28 2018, 6:04 AM

LGTM. Might want to leave a 'FIXME' note on that cost model test file. Or move anything of value in there that isn't already tested in the codegen test file.

lib/Target/X86/X86ISelLowering.cpp
35471 ↗(On Diff #175533)

Thanks - those diffs are what I imagined. It's better to trade a move for a shift (at least on paper) and reduce the dependency chain, and that's consistent with other folds.

This revision is now accepted and ready to land.Nov 28 2018, 6:04 AM
This revision was automatically updated to reflect the committed changes.