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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
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.
----------------
aditya_nandakumar wrote:
> 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? 
It's not an either-or thing, they should call both. There are two events involved in changing an instruction, the first (changingInstr) occurs before the changes are made and the second (changedInstr) occurs after the changes are made.


================
Comment at: include/llvm/CodeGen/GlobalISel/CombinerHelper.h:38
+  void replaceRegWith(MachineRegisterInfo &MRI, unsigned FromReg, unsigned ToReg) const;
+
   /// If \p MI is COPY, try to combine it.
----------------
aditya_nandakumar wrote:
> Also, should replaceRegWith respect register constraints (such as RegBank/RegClass) and emit copies while replacing?
It's not necessary for the calls in CombineHelper right now but we will certainly want a variant of this that does that at some point


================
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));
----------------
aditya_nandakumar wrote:
> I understand what this is for, but I'm not sure if these asserts will ever trigger - and if we should have these. 
Assuming we write our code correctly, they won't trigger :-).

We spoke off-list about this and it's my understanding that your main concern is that GISelWorkList should be able to handle instructions that aren't in a function+bb. We don't have any such instances right now but I see your point. We could potentially tweak the name to make it clearer or we could wait until we have a case that works on MI's that aren't in a block/function and resolve it then. I suspect we're unlikely to find such a case in the near future since it's not common to create an instruction but keep it outside of bbs/functions long term.


================
Comment at: lib/CodeGen/GlobalISel/Combiner.cpp:65
+    LLVM_DEBUG(dbgs() << "Changing: "; MI.print(dbgs()));
     WorkList.insert(&MI);
   }
----------------
This line shouldn't be here and is probably the cause of your confusion about the difference between changingInstr() and changedInstr(). It won't cause any trouble but it's not necessary.


Repository:
  rL LLVM

https://reviews.llvm.org/D52947