This is an archive of the discontinued LLVM Phabricator instance.

[X86][Nearly NFC] Split SHLD/SHRD into their own WriteShiftDouble class
ClosedPublic

Authored by lebedev.ri on Jul 6 2018, 3:02 AM.

Details

Summary


While there is still some discreteness within that new group,
it is clearly separate from the other shifts.

This is purely mechanical change, it does not change any numbers,
as the [lack of the change of] mca tests show.

I'm guessing FeatureSlowSHLD is related.

Agner's tables agree, these double shifts are clearly different
from the normal shifts/rotates.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Jul 6 2018, 3:02 AM
craig.topper accepted this revision.Jul 6 2018, 1:38 PM

I definitely believe these instructions would be different than single shifts. LGTM

This revision is now accepted and ready to land.Jul 6 2018, 1:38 PM

I definitely believe these instructions would be different than single shifts. LGTM

Thank you for the review!

Now only waiting for someone to stamp D48997 :)
It is NFC, but i do not feel confident just landing it yet.

RKSimon added inline comments.Jul 8 2018, 2:32 AM
lib/Target/X86/X86Schedule.td
145 ↗(On Diff #154381)

I'm not sure a basic sched pair is the best match for double shifts, at least on AMD targets the imm/reg versions and the RMW versions are all pretty different in their behaviours - see X86ScheduleBtVer2.td.

I'd be a lot happier if we had better perf numbers for all of these (we did some for Jaguar but nothing else - we need some decent Intel tests for starters) and then we can decide how to create the classes to match.

@lebedev.ri What do you think?

lebedev.ri added inline comments.Jul 8 2018, 2:43 AM
lib/Target/X86/X86Schedule.td
145 ↗(On Diff #154381)

As the inconsistencies.html from Piledriver/bdev2 shows, it is indeed not the best match.
But keeping it in the WriteShift is clearly not ideal either.

I'm afraid i won't be of any help with numbers for other CPU's, as i don't have those.

These things aren't set in stone, right? They can be fine-tuned later?
If not, please click "request changes" now :)

lebedev.ri edited the summary of this revision. (Show Details)

Rebased, NFC.

This revision was automatically updated to reflect the committed changes.