[PATCH] D145169: FastRegAlloc: Fix implicit operands not rewritten

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: llvm/lib/CodeGen/RegAllocFast.cpp:1410
+      useVirtReg(MI, I, Reg);
+      ChangedOperands = true;
+    }
----------------
gbossu wrote:
> arsenm wrote:
> > Don't you only need to maybe repeat if you saw a kill use?
> At the moment, that is right. I just preferred to play it safe because some of the callees inside `RegAllocFast::setPhysReg()` could potentially mess around with implicit operands further. Although, to this date, I haven't found any other case :) Tbh, it's also not easy to find out whether `useVirtReg()` has changed any implicit operands, so I preferred to be conservative.
TL;DR only redo the loop when something changes.

Could we modify `useVirtReg` to surface whether or not something changed?
It should be possible to surface `addRegisterKilled` and whatnot that `setPhysReg` uses.

Bonus point would be to differentiate when we add an operand (actually removing is the issue here) vs. when we just update one, but we can cross that bridge later if compile time is problematic.

I guess we also could be clever and instead of redoing all the iterations of the loop only tweak how the iterator moves (i.e., don't advance if there is a change), but that's probably not worth and may be being too clever for our own good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145169