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

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar added inline comments.


================
Comment at: include/llvm/CodeGen/GlobalISel/CombinerHelper.h:42
+  /// observer of the changes.
+  void replaceRegOpWith(MachineRegisterInfo &MRI, MachineOperand &FromReg,
+                        unsigned ToReg) const;
----------------
Consider naming the second parameter something similar to "FromRegOp" for better clarity.


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:39
+void CombinerHelper::replaceRegOpWith(MachineRegisterInfo &MRI,
+                                      MachineOperand &FromReg,
+                                      unsigned ToReg) const {
----------------
ditto for naming FromReg to indicate it's a MachineOperand.


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:44
+
+  FromReg.setReg(ToReg);
+
----------------
If this MachineOperand is a def, should this call replaceRegWith which also handles the constraining? 


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:320
+    Observer.changingInstr(*UseMO->getParent());
     UseMO->setReg(NewDstReg);
+    Observer.changedInstr(*UseMO->getParent());
----------------
Should this call CombinerHelper::replaceRegOpWith which will handle the Observer notifying?


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:327
   }
   MI.getOperand(0).setReg(ChosenDstReg);
+  Observer.changedInstr(MI);
----------------
This also looks like it could go through replaceRegOpWith interface?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D52947