Page MenuHomePhabricator

Adjust MIScheduler to use ProcResource counts

Authored by chill on Aug 23 2018, 6:40 AM.



This fix allows the scheduler to take into account the number of
instances of each ProcResource specified. Previously a declaration in a
scheduler of ProcResource<1> would be treated identically to a
declaration of ProcResource<2>. This patch fixes that. Testsuite
fixes are included for schedulers that have changed.

Diff Detail


Event Timeline

Jackson created this revision.Aug 23 2018, 6:40 AM

Would it be possible to add a test case that highlights the problem you are solving e.g. take a processor that has multiple resources (e.g. two adders) and show that it now can use both resources in same cycle.

1945 ↗(On Diff #162169)

why Offset is uninitialized ?

chill added inline comments.Aug 29 2018, 2:05 AM
1937 ↗(On Diff #162169)

I would write it like:

/// Compute the next cycle at which the given processor resource can be
/// scheduled.  Returns (NextCycle, InstanceIDAssignedTo).
std::pair<unsigned, unsigned> SchedBoundary::
getNextResourceCycle(unsigned PIdx, unsigned ResCount, unsigned Cycles) {
  unsigned MinNextUnreserved = InvalidCycle;
  unsigned InstanceID = 0;
  unsigned StartIndex = ProcessorResourceList[PIdx];
  unsigned NumUnits = SchedModel->getProcResource(PIdx)->NumUnits;

  for (unsigned I = StartIndex, E = StartIndex + NumUnits;  I < E; ++I) {
    unsigned NextUnreserved =  getNextResourceCycleByInstance(I, Cycles);
    if (NextUnreserved < MinNextUnreserved) {
        InstanceID = I;
        MinNextUnreserved = NextUnreserved;
  return std::make_pair(MinNextUnreserved, InstanceID);

but YMMV.

Jackson added inline comments.Aug 29 2018, 2:14 AM
1937 ↗(On Diff #162169)

Where does InvalidCycle come from here?

chill added inline comments.Aug 29 2018, 2:26 AM
1937 ↗(On Diff #162169)

line 1817

Jackson updated this revision to Diff 163088.Aug 29 2018, 7:45 AM

Thanks for the feedback. I have fixed up the getNextResourceCycle function and added a testcase.

fhahn added a subscriber: fhahn.Aug 29 2018, 8:35 AM
javed.absar added inline comments.Oct 10 2018, 1:39 PM
1 ↗(On Diff #163088)

The filename has a typo 'resouces'.
Also, it may be helpful to add a comment at the top of this file on what exactly is being tested.

chill commandeered this revision.Oct 11 2018, 8:33 AM
chill added a reviewer: Jackson.

Taking over as Jackson's internship ended in the meantime.

chill updated this revision to Diff 192220.Mar 25 2019, 4:53 PM

Updated regression tests.

Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2019, 4:53 PM
fhahn added a comment.Mar 26 2019, 3:40 AM

Do you have any benchmark data you could share with this patch? I am curious what the impact is and if there are any regressions.

686 ↗(On Diff #192220)

This name is too generic I think. this is only relevant to ReservedCycles. Also it is basically a map, right?

119 ↗(On Diff #192220)

Is there a benefit if adding this over just using getProcResource? Leaking the table may make future changes harder.

1974 ↗(On Diff #192220)

ResCount not used here? I think you can remove it, as you get it from the model in the function.

2036 ↗(On Diff #192220)

InstanceID unused?

2202 ↗(On Diff #192220)

InstanceID only used with DEBUG?

2207 ↗(On Diff #192220)

Can we convert the instanceID to a resource name?

15 ↗(On Diff #192220)

The function is quite small, can you check the full schedule?

andreadb added inline comments.Mar 26 2019, 5:00 AM
686 ↗(On Diff #192220)

I agree: the name should be improved.
ProcessorResourceList is only used to map processor resource identifiers to ReservedCycles indices. That being said, it is okay to have it as a small vector with size SchedModel->getNumProcResourceKinds().

119 ↗(On Diff #192220)


I don't think it is really needed (see my other comment below).

1930–1933 ↗(On Diff #192220)

Instead of doing (Resources + i)->NumUnits, you should use
SchedModel->getProcResource(i) to ge the MCProcResourceDesc, and then access field NumUnits.

So, something like:

NumUnits += SchedModel->getProcResource(i)->NumUnits;

That way, you can get rid of the code at line 1924. You also don't need method getProcResourceTable().

1979–1980 ↗(On Diff #192220)

We should probably enforce this constraint in the SubtargetEmitter and error out if a processor resource declares zero (or negative) units.

(In practice, none of the models available upstream declares a resource with zero units. Only the 'InvalidUnit' at index 0 defaults to having zero units).

I scanned the code. It seems reasonable to me. Thanks for all the other good review feedback.

chill updated this revision to Diff 197737.May 2 2019, 3:51 AM

Thanks for the feedback. Here's an updated diff.

chill marked 5 inline comments as done.May 2 2019, 3:52 AM
chill marked 4 inline comments as done.May 2 2019, 3:59 AM

Do you have any benchmark data you could share with this patch? I am curious what the impact is and if there are any regressions.

No, I'm sorry, I don't have publishable benchmark results at the moment.

2036 ↗(On Diff #192220)

Yes, it's unused, here and in one more place we care only for the cycle number.

15 ↗(On Diff #192220)

I'm not sure what do you mean. The test tests the difference in the internal state of the scheduler, which does not necesarily results in different schedule. Perhaps I can make a bigger test case, but I'd rather not do it as it'll have the tendency to break with unrelated scheduler changes.

chill marked 3 inline comments as done.May 2 2019, 4:00 AM
chill added inline comments.
1979–1980 ↗(On Diff #192220)

That would be for a separate patch, wouldn't it ?

andreadb accepted this revision.May 2 2019, 4:58 AM
andreadb added inline comments.
1979–1980 ↗(On Diff #192220)

Yes. That would be a follow-up patch.

You can commit this change since it is good (and it was already approved by @atrick ).


This revision is now accepted and ready to land.May 2 2019, 4:58 AM
This revision was automatically updated to reflect the committed changes.
chill marked an inline comment as done.