[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:38
+  void replaceRegWith(MachineRegisterInfo &MRI, unsigned FromReg, unsigned ToReg) const;
+
   /// If \p MI is COPY, try to combine it.
----------------
dsanders wrote:
> 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
It would be great if you can address this (even with a naive check if constraints are not met insert a copy). This can be a very subtle bug to figure out when it starts happening.


================
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));
----------------
dsanders wrote:
> 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.
Yes. Please update the comment to reflect this new change. Also if you're checking that you're inserting valid instructions (with parents), you should move these asserts to the non const method as well.


Repository:
  rL LLVM

https://reviews.llvm.org/D52947