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

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1238
                      {DestDivReg, DestRemReg},
                      {MI.getOperand(1).getReg(), MI.getOperand(2).getReg()});
   MI.eraseFromParent();
----------------
efriedma wrote:
> 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.)
Oh right, I see what you mean now. Yes I think that works, I'll check it does and revert/change this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144336