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

[Gaëtan Bossu via Phabricator via llvm-commits]

gbossu added inline comments.


================
Comment at: llvm/lib/CodeGen/RegAllocFast.cpp:1410
+      useVirtReg(MI, I, Reg);
+      ChangedOperands = true;
+    }
----------------
qcolombet wrote:
> gbossu wrote:
> > qcolombet wrote:
> > > 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.
> > Sorry it took me some time to get back to this patch. I changed a couple of functions to propagate whether kill/dead flags were added, in which case I assume the list of MOs might have been re-arranged.
> > 
> > I cannot say I'm 100% happy about the change, there are lots of nesting levels in RegAllocFast, and I'm essentially adding a new one. I could move some more logic in separate functions (like I did for `findAndSortDefOperandIndexes`) to help readability if needed. So far I tried to strike a balance between keeping the code readable and also not refactor too much. Suggestions are welcome for further improvements.
> Let's go with that for now
Great, thanks for the review. I believe I still do not have "write" permission. Could you maybe commit on my behalf? Here are my details: `Gaëtan Bossu <gaetan.bossu at amd.com>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145169