Page MenuHomePhabricator

[SchedModel] Complete models shouldn't match against itineraries when they don't use them (PR35639) (WIP)
ClosedPublic

Authored by RKSimon on Feb 13 2018, 6:21 AM.

Details

Summary

For schedule models that don't use itineraries, checkCompleteness still checks that an instruction has a matching itinerary instead of skipping and going straight to matching the InstRWs. That doesn't seem to match what happens in TargetSchedule.cpp

This patch causes problems for a number of models flagged as complete that I've included for reference/discussion.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.Feb 13 2018, 6:21 AM

Ah, so this is a problem when some processor models use itineraries and others don't? In that case, the change looks good to me ...

I was under the impression that the purpose of ItinRW is a way to map processor resources to itineraries that were already defined. This in turn prevents the need to update existing instructions (since they have been defined with a list of itineraries).

Judging by this patch and the need to set the PPC P9 scheduling model to incomplete seems to suggest that this understanding is incorrect.

I can confirm that the purpose of ItinRW is to avoid the need to define InstRW for a subtarget when the target already had itinerary classes. (Things would be *a lot* simpler if all the subtargets used the same style of machine description.)

CompleteModel can mean whatever is currently most useful to devs, but we don't want to lose the scheduling-time checks just because a subtarget doesn't define InstRW for everything. Maybe it would work for those subtargets should have InstRW { let Unsupported = 1 } for opcodes that aren't covered by itinerary classes?

I can confirm that the purpose of ItinRW is to avoid the need to define InstRW for a subtarget when the target already had itinerary classes. (Things would be *a lot* simpler if all the subtargets used the same style of machine description.)

CompleteModel can mean whatever is currently most useful to devs, but we don't want to lose the scheduling-time checks just because a subtarget doesn't define InstRW for everything. Maybe it would work for those subtargets should have InstRW { let Unsupported = 1 } for opcodes that aren't covered by itinerary classes?

I am not sure I completely follow what you're suggesting. I'll use Power9 scheduling model in the PPC back end as an example.
We've implemented the model to try to be as accurate as we can. A vast majority of instructions that were defined with the same itinerary use the same functional units, so we just used ItinRW to map instructions with a specific itinerary to specific functional units. For instructions that don't have an itinerary, we've provided an InstRW or set the "no scheduling information" bit. Also, for instructions that have an itinerary, but use different functional units from most other instructions with that itinerary, we've overridden the resources that come from ItinRW with InstRW. This has allowed us to mark the model complete.

Since all of our instructions either had an itinerary specified or have been covered in InstRW (and all our itineraries covered by ItinRW), we assumed that all our instructions have all the processor resources correctly specified. However, judging by this patch and the fact that we don't satisfy the condition on line 1637 (!SC.Writes.empty()), it appears that we actually misunderstood what ItinRW actually does. In fact, it appears to me that a vast majority of our instructions (not covered by InstRW) don't actually provide any information to the scheduler about which functional resources in the CPU they consume. And if so, I am of course very much in favour of this patch as it highlights a problem we didn't know we had.

Is this an accurate understanding? If so, I'm not sure what the actual semantics of ItinRW are. Also, if we have to write InstRW's for all our instructions in our case, we don't mind doing that. I just want to make sure that's what we should be doing before spending time on it.

fhahn added a subscriber: fhahn.Feb 27 2018, 12:44 AM
RKSimon updated this revision to Diff 136792.Mar 2 2018, 10:31 AM

Removed Cortex-A57 model diff now that rL326304 has landed

With the patch rL327021 we should now be able to mark Power 9 as complete.

RKSimon updated this revision to Diff 137636.Mar 8 2018, 12:23 PM
RKSimon edited the summary of this revision. (Show Details)

Removed Power9 model diff now that rL327021 has landed

Ping - is anyone wanting to fix the SI or MIPs scheduler models to be complete again before I commit this? @sdardis @arsenm

@RKSimon , I'm ok with this going in, I'm in process of updating our scheduler models to account for this but it may take me a few days.

andreadb accepted this revision.Apr 5 2018, 3:55 AM

ping^2

Looks good to me.

Thanks!

This revision is now accepted and ready to land.Apr 5 2018, 3:55 AM
This revision was automatically updated to reflect the committed changes.