This is an archive of the discontinued LLVM Phabricator instance.

[X86] Fix skylake server scheduling info.
ClosedPublic

Authored by courbet on Jun 4 2018, 8:26 AM.

Details

Summary

This fixes most of the scheduling info for SKX vector operations.
I had to split a lot of the YMM/ZMM classes into separate classes for YMM and ZMM.

I've left out the DQ/QQ issues for now. I'll fix them in a separate change.
There seems to be several cases of broadcast/permutations specialized on the mask value, I have not touched these.

The before/after llvm-exegesis analysis are in the phabricator diff.

Diff Detail

Repository
rL LLVM

Event Timeline

courbet created this revision.Jun 4 2018, 8:26 AM
RKSimon added a subscriber: RKSimon.

Please can you pull the unsupported diff for existing classes out into a separate patch for inital review? This patch can then be just about the changes for SkylakeServer

Please can you pull the unsupported diff for existing classes out into a separate patch for inital review? This patch can then be just about the changes for SkylakeServer

Sure, will do.

courbet planned changes to this revision.Jun 11 2018, 1:39 AM
courbet edited the summary of this revision. (Show Details)Jun 11 2018, 5:00 AM
courbet updated this revision to Diff 150713.Jun 11 2018, 5:02 AM
  • Use the new WriteResUnsupported multiclasses
  • More sched fixes
courbet added a comment.EditedJun 11 2018, 5:02 AM

Before:

After:

courbet updated this revision to Diff 150715.Jun 11 2018, 5:08 AM
  • remove more comment noise
courbet updated this revision to Diff 150716.Jun 11 2018, 5:32 AM
  • split SKXWriteResGroup93.
  • split SKXWriteResGroup50

@RKSimon I think this is ready for review now.

Nice! A couple of minors - but almost there I reckon.

lib/Target/X86/X86SchedSkylakeServer.td
337 ↗(On Diff #150715)

You results suggest WritePHMINPOS should be just Port0?

378 ↗(On Diff #150715)

The QQ conversions might be best off with instrw overrides?

390 ↗(On Diff #150715)

The QQ conversions might be best off with instrw overrides?

406 ↗(On Diff #150715)

WriteCvtPH2PSY is the one that does pass your tests - it's WriteCvtPH2PS and WriteCvtPH2PSZ that are irregular?

609 ↗(On Diff #150715)

Worth pulling the LAHF/SAHF change out into its own patch as IIRC a lot of Intel models have dodgy values for this?

courbet updated this revision to Diff 150723.Jun 11 2018, 5:47 AM
courbet marked an inline comment as done.
  • address review comments

Thanks !

lib/Target/X86/X86SchedSkylakeServer.td
378 ↗(On Diff #150715)

I guess so. I'll do it in a separate commit as there are already a lot of changes here :(

406 ↗(On Diff #150715)

Thanks for the catch. This has been addressed indeed. The other two are a tiny bit irregular but nothing crazy - I think it's just noise.

609 ↗(On Diff #150715)

Will do.

courbet updated this revision to Diff 150725.Jun 11 2018, 5:49 AM
  • revert LAHF/SAHF changes
Harbormaster completed remote builds in B19161: Diff 150725.
RKSimon accepted this revision.Jun 11 2018, 6:49 AM

LGTM with a few final minor corrections.

lib/Target/X86/X86SchedSkylakeServer.td
978 ↗(On Diff #150725)

Keep these aligned

1091 ↗(On Diff #150725)

Merge this into "(V?)CVTSD2SS(Z?)rr" ?

1394 ↗(On Diff #150725)

Don't use instregex unless you really need to - it take a lot longer than a basic instrs match. All of these look like they will have a single match?

This revision is now accepted and ready to land.Jun 11 2018, 6:49 AM
courbet marked 3 inline comments as done.Jun 11 2018, 7:13 AM

Thanks a lot for the review.

courbet updated this revision to Diff 150749.Jun 11 2018, 7:23 AM
  • update MCA test
  • update sched tests
courbet updated this revision to Diff 150751.Jun 11 2018, 7:32 AM

More test updating.

courbet updated this revision to Diff 150752.Jun 11 2018, 7:38 AM

More test updating.

This revision was automatically updated to reflect the committed changes.