[PATCH] D52947: [globalisel][combiner] Make the CombinerChangeObserver a MachineFunction::Delegate

[Daniel Sanders via Phabricator via llvm-commits]

dsanders marked 8 inline comments as done.
dsanders added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:320
+    Observer.changingInstr(*UseMO->getParent());
     UseMO->setReg(NewDstReg);
+    Observer.changedInstr(*UseMO->getParent());
----------------
aditya_nandakumar wrote:
> Should this call CombinerHelper::replaceRegOpWith which will handle the Observer notifying?
I think this can be done. I just need to re-test it


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:327
   }
   MI.getOperand(0).setReg(ChosenDstReg);
+  Observer.changedInstr(MI);
----------------
aditya_nandakumar wrote:
> This also looks like it could go through replaceRegOpWith interface?
This one is a little special. We reported it changing right back at the start when we changed the opcode and changing/changed should probably be balanced. For the constraints aspect of it, I don't believe we need to do anything here as G_*LOAD and G_TRUNC/G_*EXT should have compatible result constraints by definition. If that's not true, the right thing to do would be to reject the combine.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D52947