This is an archive of the discontinued LLVM Phabricator instance.

[X86] Explicitly mark unsupported zmm classes in scheduling models.
ClosedPublic

Authored by courbet on Jun 5 2018, 1:11 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

courbet created this revision.Jun 5 2018, 1:11 AM

Note that this breaks scheduler tests for "GENERIC" , since we're using SNB. Is that fine ? Is there any situation where we would emit AVX512 but still use generic to schedule ?

courbet updated this revision to Diff 149966.Jun 5 2018, 6:07 AM
  • add unit tests for reference

Note that this breaks scheduler tests for "GENERIC" , since we're using SNB. Is that fine ? Is there any situation where we would emit AVX512 but still use generic to schedule ?

Sorry it's not fine. Specifying by ISA feature attribute and not cpu model is a common occurrence. SNB needs to "support" all scheduler classes.

My recommendation from D47721 was to not introduce any new classes in this new patch - it should purely be about flagging unsupported existing classes on each model. D47721 would then be about adding the new ZMM classes.

lib/Target/X86/X86SchedSandyBridge.td
99 ↗(On Diff #149911)

SNB is our generic model - we can't flag it as not supporting ANY instruction. Remove these entries and leave a note explaining why.

Note that this breaks scheduler tests for "GENERIC" , since we're using SNB. Is that fine ? Is there any situation where we would emit AVX512 but still use generic to schedule ?

Sorry it's not fine. Specifying by ISA feature attribute and not cpu model is a common occurrence. SNB needs to "support" all scheduler classes.

That's what I was afraid of :(
How do we choose which scheduling info we want to use for SNB for unsupported instructions ? Do we just come up with something random ?

My recommendation from D47721 was to not introduce any new classes in this new patch - it should purely be about flagging unsupported existing classes on each model.

OK: so each model but SNB, right ?

courbet updated this revision to Diff 149978.Jun 5 2018, 7:20 AM

[X86] Explicitly mark unsupported zmm classes in scheduling models.

courbet retitled this revision from [X86][NFC] Create zmm sched classes. to [X86] Explicitly mark unsupported zmm classes in scheduling models..Jun 5 2018, 7:20 AM

Now there's the question of what we do with KNL, which has AVX512 but uses the HaswellModel. Opinions ?

RKSimon added inline comments.Jun 5 2018, 7:40 AM
lib/Target/X86/X86SchedHaswell.td
144 ↗(On Diff #149978)

This is going to be a problem for KNL.......

lib/Target/X86/X86SchedSandyBridge.td
104 ↗(On Diff #149978)

Keep the comment, but I'd probably recommend just leaving these in their original places (possibly with an "// Unsupported" next to it?) for comparison to "supported" cases - I typically just copied the YMM values for the ZMM cases - not great but it at least had consistency........

lib/Target/X86/X86ScheduleAtom.td
83 ↗(On Diff #149978)

There are a lot more than this that aren't supported in this model - move them too?

lib/Target/X86/X86ScheduleBtVer2.td
162 ↗(On Diff #149978)

There are a lot more than this that aren't supported in this model - move them too?

lib/Target/X86/X86ScheduleSLM.td
82 ↗(On Diff #149978)

There are a lot more than this that aren't supported in this model - move them too?

RKSimon added inline comments.Jun 5 2018, 8:03 AM
lib/Target/X86/X86SchedHaswell.td
144 ↗(On Diff #149978)

Until KNL moves to its own model (PR26418) the HSW model is going to have to be left in a similar state to SNB,

courbet updated this revision to Diff 150087.Jun 6 2018, 1:43 AM
courbet marked 6 inline comments as done.

Address review comments, mark more unsupported classes in {BtVer2,Atom,SLM}

What do you think about adding X86WriteUnsupported and X86WriteUnsupportedResPair to X86Schedule.td (similar to D47766) - that way we can keep the unsupported classes in place (next to the XMM/YMM versions) - they might be easy to track then.

That's something I was considering after the discussion on the other thread. I'll update the diff.

courbet updated this revision to Diff 150296.Jun 7 2018, 4:31 AM

Introduce X86WriteRes(|Pair)Unsupported.

avt77 added a subscriber: avt77.Jun 7 2018, 6:35 AM

Introduce X86WriteRes(|Pair)Unsupported.

I like these Unsupported.

RKSimon accepted this revision.Jun 8 2018, 6:20 AM

LGTM with one minor - thanks.

lib/Target/X86/X86ScheduleSLM.td
429 ↗(On Diff #150296)

The others in this block can be converted as well.

This revision is now accepted and ready to land.Jun 8 2018, 6:20 AM
courbet updated this revision to Diff 150678.Jun 11 2018, 12:01 AM
courbet marked an inline comment as done.

Mark more unsupported classes in SLM.

This revision was automatically updated to reflect the committed changes.