[PATCH] D103951: [NFC][Scheduler] Refactor tryCandidate to return boolean

[Qiu Chaofan via Phabricator via llvm-commits]

qiucf marked an inline comment as done.
qiucf added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineScheduler.cpp:3191
   // Bias PhysReg Defs and copies to their uses and defined respectively.
   if (tryGreater(biasPhysReg(TryCand.SU, TryCand.AtTop),
                  biasPhysReg(Cand.SU, Cand.AtTop), TryCand, Cand, PhysReg))
----------------
foad wrote:
> qiucf wrote:
> > foad wrote:
> > > 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;
> > > ```
> > 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.
> 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.
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) {
  // ...
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103951/new/

https://reviews.llvm.org/D103951