Page MenuHomePhabricator

[X86] Improve 64-bit shifts on 32-bit targets (PR14593)
ClosedPublic

Authored by RKSimon on Jul 31 2016, 8:13 AM.

Details

Summary

As discussed on PR14593, this patch adds support for lowering to SHLD/SHRD from the patterns generated by DAGTypeLegalizer::ExpandShiftWithKnownAmountBit.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon updated this revision to Diff 66243.Jul 31 2016, 8:13 AM
RKSimon retitled this revision from to [X86] Improve 64-bit shifts on 32-bit targets (PR14593).
RKSimon updated this object.
RKSimon added reviewers: eli.friedman, bkramer, spatel.
RKSimon set the repository for this revision to rL LLVM.
RKSimon added a subscriber: llvm-commits.
eli.friedman edited edge metadata.Jul 31 2016, 8:51 AM

Might be nice to have a test which triggers this combine without going through legalization. (Should be possible, I think?)

lib/Target/X86/X86ISelLowering.cpp
28675

Maybe it makes sense to split this into two separate if statements; one checking that the two outer shifts match, one checking that the inner shift is a shift by one. I found it a bit tricky to pick through. (I sort of wish we had an equivalent to IR pattern matching in SelectionDAG.)

majnemer added inline comments.
lib/Target/X86/X86ISelLowering.cpp
28675

I actually worked on such a thing: http://pastebin.com/raw/A7WfuVC0

Think it makes sense for me to return to it?

RKSimon updated this revision to Diff 66245.Jul 31 2016, 11:07 AM
RKSimon edited edge metadata.

Updated based on Feedback

RKSimon added inline comments.Jul 31 2016, 11:13 AM
lib/Target/X86/X86ISelLowering.cpp
28675

Possibly - I'm interested in being able to match a lot of DAGCombiner patterns for both scalar and vector types.

Currently, at best we only match vector types with 'splatted' constant values - being able to generalize this would be very useful, I've played with some basic all_of style approaches but nothing has been great.

Could your template patterns make this any easier?

eli.friedman accepted this revision.Jul 31 2016, 12:15 PM
eli.friedman edited edge metadata.

LGTM.

lib/Target/X86/X86ISelLowering.cpp
28679

If DAGCombine doesn't transform Y+Y to Y<<1, it should. (instcombine definitely does this.) Not really important for now, though.

This revision is now accepted and ready to land.Jul 31 2016, 12:15 PM

Thanks Eli.

lib/Target/X86/X86ISelLowering.cpp
28679

In the old codegen we were seeing code like 'leal (%edx,%edx), %eax' being lowered from shl( v, 1 ) IR.

eli.friedman added inline comments.Jul 31 2016, 12:49 PM
lib/Target/X86/X86ISelLowering.cpp
28679

There's a pattern in X86InstrCompiler.td which does a shift->add transform because it's more efficient on x86... but we want to keep the SelectionDAG in canonical form as long as possible.

This revision was automatically updated to reflect the committed changes.