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

[Daniel Sanders via Phabricator via llvm-commits]

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


================
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:
> 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.
I believe the only remaining review comment is this one. I'm not sure what change you're referring to here w.r.t the comment to be updated (probably because it's taken so long for me to get back to this, sorry).

The asserts are only necessary where it's not clear that you have the right to modify the instructions which is only an issue for the const version. The non-const version is clearly allowed to modify it as it's not const whereas the const version has to know it can find a non-const pointer to the same object.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D52947