[PATCH] D88256: [NFC][regalloc] Separate iteration from AllocationOrder

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: llvm/lib/CodeGen/RegAllocGreedy.cpp:811
   Register PhysReg;
-  while ((PhysReg = Order.next())) {
-    if (PhysReg == PrevReg)
+  for (auto I = Order.begin(), E = Order.end(); I != E && !PhysReg; ++I) {
+    if (*I == PrevReg)
----------------
mtrofin wrote:
> qcolombet wrote:
> > Could we just stick to assigning PhysReg at the beginning of the loop?
> > That way we don't have to modify the body of the loop.
> There's a slight nuance:
> 
> In the old code, suppose we never hit line 819 (the `break`). Then line 821 will have a 0 PhysReg (indicating loop completed, and no reg was found)
> 
> In the new code, if we just initialize PhysReg on line 812, then we'll never have a 0 PhysReg on line 826 (ok, not never - because it could be the loop is never entered).
> 
> We could have `I` in the outer scope, and check both on line 826; or - the path I preferred - we clarify that the loop searches for a value for PhysReg.
> 
> wdyt?
Sounds fine.
Let's stick to the version in that patch.


================
Comment at: llvm/lib/CodeGen/RegAllocGreedy.cpp:1860
   unsigned BestCand = NoCand;
-  Order.rewind();
-  while (unsigned PhysReg = Order.next()) {
+  for (auto PhysReg : Order) {
+    assert(PhysReg);
----------------
mtrofin wrote:
> qcolombet wrote:
> > Could you call out the type here?
> > Put differently, I don't like the use of `auto` when the type is not immediately obvious.
> Done - also changed the type of operator*, in light of our discussion - we could type it as MCPhysReg, but I think MCRegister would also be correct, and help reduce the concept count - wdyt?
I have a slight preference with MCPhysReg, but MCRegister is fine too.
That said, it is strange to have `ArrayRef<MCPhysReg> getOrder` returning a different type.
I would rather be consistent, but I can live with the iterator offering the richer type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88256