Page MenuHomePhabricator

[NFC][Scheduler] Refactor tryCandidate to return boolean

Authored by qiucf on Jun 9 2021, 3:07 AM.



This patch changes return type of tryCandidate from void to bool. Because:

  • Method in some targets already follows this convention. (SIMachineScheduler)
  • This will help if some target wants to re-use generic code. (see PowerPC change part)
  • This looks more intuitive - why tryLess/tryGreater returns bool while tryCandidate returns nothing?

Here all branches simply return true if they got compared. We may change this logic further: return true only if TopCand.Reason is set (not NoCand anymore), so that we can:

if (tryCandidate(Cand, TopCand, nullptr)) {
  // ...

Any comments are welcome. Thanks.

Diff Detail

Event Timeline

qiucf created this revision.Jun 9 2021, 3:07 AM
qiucf requested review of this revision.Jun 9 2021, 3:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2021, 3:07 AM

We may change this logic further: return true only if TopCand.Reason is set (not NoCand anymore)

This seems to be better. Returning true with NoCand is misleading to me.

foad added inline comments.Jun 11 2021, 1:56 AM

Update the comment to explain what the return value means. Why don't you return true when you set the reason to NodeOrder?


Couldn't you do this refactoring anyway, even without changing the return type? E.g.

GenericScheduler::tryCandidate(Cand, TryCand, Zone);
if (TryCand.Reason != NodeOrder && TryCand.Reason != NoCand)
qiucf updated this revision to Diff 352939.Jun 18 2021, 1:57 AM
qiucf marked 2 inline comments as done.

Return false only if TryCand is NoCand

I have no objections.

foad added inline comments.Jun 22 2021, 1:39 AM

Surely all of these "try" functions should consistently only return true when they set Reason to something useful? So all these clauses should just be:

if (trySomething(...))
  return true;
qiucf added inline comments.Jun 28 2021, 2:35 AM

I think current implementation is for saving compilation time (tryCandidate should be very hot):

  • If TryCand is better than Cand, return true;
  • If Cand is better than TryCand, it also returns true, since there's no need to execute comparisons below;
  • Only if we don't know which is better, go next check.
/// Return true if this heuristic determines order.
bool tryLess(int, int, SchedCandidate, SchedCandidate, CandReason);

(return true if order determined vs. return true if TryCand is better)

So the proposed change looks better, but seems it'll increase compilation time.

foad accepted this revision.Jun 28 2021, 5:48 AM
foad added inline comments.

Thanks for explaining. My suggestion wouldn't just increase compilation time, it would be incorrect, because if this call to tryGreater decided that Cand is better than tryCand, we would ignore that and allow a later (lower priority) heuristic to make the decision instead.

So I think your patch is OK but I still find these APIs confusing. Maybe as a future change, tryLess and tryGreater could return -1, 0 or 1 like operator<=> does.

This revision is now accepted and ready to land.Jun 28 2021, 5:48 AM
qiucf marked an inline comment as done.Jun 30 2021, 11:31 PM
qiucf added inline comments.

Yes. Maybe like:

enum TryResult {
  CandBetter = -1,
  Equal = 0,
  TryCandBetter = 1,

TryResult tryLess(...) {}

So we can write like this:

TryResult tryCandidate(...) {
  if (auto result = tryLess(...))
    return result;
  // ...

if (tryCandidate(...) == TryCandBetter) {
  // ...
This revision was landed with ongoing or failed builds.Jun 30 2021, 11:36 PM
This revision was automatically updated to reflect the committed changes.
qiucf marked an inline comment as done.