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

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar added a comment.

Hi Daniel, the patch looks mostly good - I just have a couple of minor nitpicks.



================
Comment at: include/llvm/CodeGen/GlobalISel/Combiner.h:49
+  virtual void changedInstr(const MachineInstr &MI) = 0;
+
+  /// All the instructions using the given register are being changed.
----------------
It's not very clear from the comments regarding when the clients using the observer should call changingInstr vs changedInstr. What happens if when they are changing instruction, and they forget to call changinginst and only call changedInst? 


================
Comment at: include/llvm/CodeGen/GlobalISel/GISelWorkList.h:54
+    // But don't actually do the search since we can derive it from the const
+    // pointer.
+    insert(const_cast<MachineInstr *>(I));
----------------
I understand what this is for, but I'm not sure if these asserts will ever trigger - and if we should have these. 


Repository:
  rL LLVM

https://reviews.llvm.org/D52947