This is an archive of the discontinued LLVM Phabricator instance.

[X86] Update BTVER2 sched numbers for some AVX instructions (xmm version)
ClosedPublic

Authored by avt77 on Nov 15 2017, 1:49 AM.

Details

Summary

The current sched model does not have proper numbers for many128-bit instructions. This patch is the first one which tries to fix this issue.

Diff Detail

Event Timeline

avt77 created this revision.Nov 15 2017, 1:49 AM
avt77 retitled this revision from Update of sched numbers for some mmx version of AVX instructions on btver2 to [X86] Update BTVER2 sched numbers for some AVX instructions (mmx version).Nov 23 2017, 1:56 AM
RKSimon edited edge metadata.Nov 25 2017, 7:49 AM

We're adding a lot of instregex exntries just to get around the poor quality of the sched classes in X86Schedule.td - better to fix them?

lib/Target/X86/X86ScheduleBtVer2.td
602–613

Shouldn't stores (mr) have a JSAGU dependency not a JLAGU

607

Shouldn't stores (mr) have a JSAGU dependency not a JLAGU

612

Shouldn't stores (mr) have a JSAGU dependency not a JLAGU?

621

Remove ResourceCyles? Looks like the default.

623

I wonder if we'd be better off introducing WriteVecLoad/WriteVecStore/WriteVecMove sched rw classes like the FIXME comment at line 140 suggests?

627

Shouldn't stores (mr) have a JSAGU dependency?

629

Should JLAGU be first for a load?

631

Remove ResourceCyles? Looks like the default.

649

Would we be better off introducing a WriteFCmp sched rw class to cover max/min and cmp?

708

Don't include commented out lines.

714

Don't include commented out lines.

avt77 updated this revision to Diff 124360.Nov 27 2017, 5:16 AM

I fixed all Simon's notes/comments/requirements.

avt77 added a comment.EditedNov 28 2017, 11:13 PM

We're adding a lot of instregex exntries just to get around the poor quality of the sched classes in X86Schedule.td - better to fix them?

It seems I missed this comment: what do you mean here? I suppose it is what you wrote in PR17367. Am I right? If yes I could try to make the first steps in this direction but obviously the patch will increase in this case because:

  • we're going to add the common classes;
  • as result we'll be forced to fix several models simulteniously.

Is it OK?

avt77 retitled this revision from [X86] Update BTVER2 sched numbers for some AVX instructions (mmx version) to [X86] Update BTVER2 sched numbers for some AVX instructions (xmm version).Dec 6 2017, 10:40 PM
RKSimon added inline comments.Dec 11 2017, 10:12 AM
lib/Target/X86/X86ScheduleBtVer2.td
592

The MULPS versions don't seem to be necessary - they match the WriteFMul defaults - its just the MULPD versions we need.

610

Please can you add the SSE4A MOVNTSDmr/MOVNTSSmr cases here? Note they don't have a V prefix.

620

Are the moves doing anything at all? I didn't see any changes to the schedule tests. If not just remove this

avt77 updated this revision to Diff 126958.Dec 14 2017, 7:36 AM

It seems all requirements from Simon were satisfied.

avt77 updated this revision to Diff 127303.Dec 17 2017, 11:30 PM

I removed all changes related to Atom.

avt77 updated this revision to Diff 127875.Dec 21 2017, 6:31 AM

I removed everything related to WriteVecMove and WriteVecStore.

avt77 updated this revision to Diff 128132.Dec 25 2017, 3:30 AM

I changed lattency for MOVSS/MOVSD and MOVAPS/MOVAPD and MOVDQA/MOVDQU.

avt77 added a comment.Jan 10 2018, 6:34 AM

RKSimon, do you like to see here anything more?

avt77 updated this revision to Diff 129645.Jan 12 2018, 9:36 AM

I removed WriteVecLoad as RKSimon required: it did not have any influence on cost numbers.

RKSimon accepted this revision.Jan 12 2018, 10:09 AM

LGTM - thanks

This revision is now accepted and ready to land.Jan 12 2018, 10:09 AM
This revision was automatically updated to reflect the committed changes.