This is an archive of the discontinued LLVM Phabricator instance.

[X86] Improved sched models for X86 SHLD/SHRD* instructions
ClosedPublic

Authored by avt77 on Jul 20 2018, 11:39 AM.

Details

Summary

This patch improves sched models for SHLD/SHRD* X86 instrs removing unnecessary redefinitions of instr infos. This patch is based on results of D48222.

Diff Detail

Event Timeline

avt77 created this revision.Jul 20 2018, 11:39 AM
RKSimon added inline comments.Jul 20 2018, 1:36 PM
lib/Target/X86/X86SchedBroadwell.td
109

You should be able to use the X86WriteRes multiclass for all of these (in all models)

1308

Don't leave commented out entries

lib/Target/X86/X86ScheduleSLM.td
107

Can you fix the test changes?

lib/Target/X86/X86ScheduleZnver1.td
179

Why not used like the other models?

avt77 updated this revision to Diff 157029.Jul 24 2018, 7:18 AM

I fixed all requirements from Simon - even the SLM test was fixed.

lebedev.ri added inline comments.Jul 24 2018, 7:28 AM
lib/Target/X86/X86InstrShiftRotate.td
655

Nitpicking: i wish there was some documented naming scheme.
E.g., why is this c instead of CL?

686
} // SchedRW
730–732
} // SchedRW
} // Constraints = "$src = $dst"
761
} // SchedRW
RKSimon added inline comments.Jul 24 2018, 8:24 AM
lib/Target/X86/X86Schedule.td
148–151

Can WriteShiftDouble be removed? Maybe move the new classes down here with the other shifts?

avt77 updated this revision to Diff 157466.Jul 26 2018, 5:37 AM

WriteShiftDouble was removed, other comments were fixed.

lebedev.ri accepted this revision.Jul 26 2018, 5:43 AM

No further notes from me, but please wait for @RKSimon.

This revision is now accepted and ready to land.Jul 26 2018, 5:43 AM
RKSimon accepted this revision.Jul 30 2018, 4:25 AM

LGTM with a couple of minors - cheers

lib/Target/X86/X86Schedule.td
148

Keep the // Double shift instructions. comment here

lib/Target/X86/X86ScheduleAtom.td
153

No need for the 32-bit comment - we don't do this for any other override cases.

avt77 added inline comments.Jul 30 2018, 6:09 AM
lib/Target/X86/X86Schedule.td
148

But I removed

defm WriteShiftDouble : X86SchedWritePair;

Why do we need the comments?

lib/Target/X86/X86ScheduleAtom.td
153

Are you sure? For other CPUs we have WriteSHDrri, WriteSHDrrcl,etc. for all sizes but in Atom it covers 32-bit version only. All other sizes have special redefinitions. To make this clear I put here this comment. Are you sure I have to remove it?

RKSimon added inline comments.Jul 30 2018, 6:15 AM
lib/Target/X86/X86Schedule.td
148

Aren't WriteSHD double shift instructions ?

lib/Target/X86/X86ScheduleAtom.td
153

Yes I'm sure.