[PATCH] D144336: [GlobalISel] Fix DIVREM combine from inserting a divrem before its operands' defs.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1238
                      {DestDivReg, DestRemReg},
                      {MI.getOperand(1).getReg(), MI.getOperand(2).getReg()});
   MI.eraseFromParent();
----------------
aemerson wrote:
> efriedma wrote:
> > The reason this breaks is that you're using `MI.getOperand(1).getReg()`, and that doesn't dominate the insertion position, right?  But `OtherMI->getOperand(1).getReg()` is equivalent, and if we're inserting at the position of OtherMI, it should dominate the insert position, I think?
> Might have misunderstood your comment, but we're not necessarily inserting at OtherMI. We have to insert at whichever of the pair is first in block order to not break use-defs. If any def of second's sources are dominated by the first, then there's no safe place to choose (without more careful analysis).
We're inserting at either MI or OtherMI, right?  If we're inserting at OtherMI, the operands of OtherMI dominate the insert point, so we can use the operands of OtherMI. If we're inserting at MI, the operands of MI dominate the insert point, so we can use the operands of MI.  (The operands of MI are equivalent to the operands of OtherMI, so we can choose whichever operands make the dominance work.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144336