[PATCH] D91244: [GlobalISel] Add missing operand update when copy is required

[Gabriel Hjort Åkerlund via Phabricator via llvm-commits]

ehjogab added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/Utils.cpp:76-84
     if (GISelChangeObserver *Observer = MF.getObserver()) {
       if (!RegMO.isDef()) {
         MachineInstr *RegDef = MRI.getVRegDef(Reg);
         Observer->changedInstr(*RegDef);
       }
       Observer->changingAllUsesOfReg(MRI, Reg);
       Observer->finishedChangingAllUsesOfReg();
----------------
arsenm wrote:
> I think I misread this the first time through and thought this was replacing uses. However I'm still a bit confused. 
> 
> This calls changingAllUsesOfReg after the potential change from constrainRegToClass (this will also happen even if constrainRegToClass ends up being a no-op when the register already was a compatible class). I'm further confused since the existing cases get away without calling the observer. I'm guessing this is because this is already called in contexts changing the instruction, or not bothering with the observer? If we don't need to call the observer here, I would prefer the first version but I'm unclear on whether it's really needed here at all
This functions may replace both uses as well as defs, and the bug this patch was originally intended to fix concerned lack of update for defs.

I agree that things are not clear with the observer is intended to be called and when it is strictly necessary to do so. It would be good to have someone with such knowledge to have a look at this patch. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91244