This is an archive of the discontinued LLVM Phabricator instance.

Adjust MIScheduler to use ProcResource counts
ClosedPublic

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

Details

Summary

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.

lib/CodeGen/MachineScheduler.cpp
1945 ↗(On Diff #162169)

why Offset is uninitialized ?

chill added inline comments.Aug 29 2018, 2:05 AM
lib/CodeGen/MachineScheduler.cpp
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
lib/CodeGen/MachineScheduler.cpp
1937 ↗(On Diff #162169)

Where does InvalidCycle come from here?

chill added inline comments.Aug 29 2018, 2:26 AM
lib/CodeGen/MachineScheduler.cpp
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
test/CodeGen/ARM/proc_resouces_schedule.ll
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.

llvm/include/llvm/CodeGen/MachineScheduler.h
686

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

llvm/include/llvm/CodeGen/TargetSchedule.h
119

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

llvm/lib/CodeGen/MachineScheduler.cpp
1974

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

2036

InstanceID unused?

2202

InstanceID only used with DEBUG?

2207

Can we convert the instanceID to a resource name?

llvm/test/CodeGen/ARM/proc-resource-sched.ll
15

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

andreadb added inline comments.Mar 26 2019, 5:00 AM
llvm/include/llvm/CodeGen/MachineScheduler.h
686

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().

llvm/include/llvm/CodeGen/TargetSchedule.h
119

+1.

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

llvm/lib/CodeGen/MachineScheduler.cpp
1930–1933

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

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.

llvm/lib/CodeGen/MachineScheduler.cpp
2036

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

llvm/test/CodeGen/ARM/proc-resource-sched.ll
15

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.
llvm/lib/CodeGen/MachineScheduler.cpp
1979–1980

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

andreadb accepted this revision.May 2 2019, 4:58 AM
andreadb added inline comments.
llvm/lib/CodeGen/MachineScheduler.cpp
1979–1980

Yes. That would be a follow-up patch.

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

Thanks,
Andrea

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.