[PATCH] D81899: [gicombiner] Unify common state for current targets into CommonTargetCombinerHelperState

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h:278-279
+protected:
+  GISelChangeObserver &Observer;
+  MachineIRBuilder &B;
+  CombinerHelper &Helper;
----------------
dsanders wrote:
> arsenm wrote:
> > I've been trying to sort out the relationship between MachineIRBuilder, *Helper, and observers recently. What is the relationship between the builder and observer here? Is it expected the builder contains this observer, or is this observer expected to also be an observer in the builder?
> Helpers need to tell observers what they did so that the enclosing algorithm can maintain its state (e.g to forget erased instructions). For convenience of implementation, Helpers rely on the MachineFunction delegate to report create/erase. The Helper must report about-to-change/changed directly to the observer.
> 
> Something must inform the CSE MIR Builder about invalidated state. It doesn't matter too much whether it's the MachineFunction delegate or the MachineIRBuilder that does this so long as it always happens for every instruction create/delete (the delegate is preferred if instrs can be built without the MachineIRBuilder but otherwise it's equivalent). There was a point where we didn't have the observer that could forward to multiple observers. That could explain why MachineIRBuilder has an observer as well. IIRC, there was also a CSE MIR Builder that could only CSE things the builder made at one point.
> 
> So in summary, the algorithms observer and the LLVM_DEBUG() printing observer (which may be the same one) must:
> * Given to the Helper
> * If MachineIRBuilder is the only way instructions are created: Registered with either the MachineFunction delegate or the MachineIRBuilder
> * Otherwise: Registered with the MachineFunction delegate
> 
> and the CSE MIR Builders observer must:
> * Be registered with either the MachineFunction delegate or the MachineIRBuilder
I am thinking we should simplify this and make sure the MachineFunction has the observer installed before we call any hooks. Observers could then be accessed through MF.getObserver().
In other words,
1) MF always has observer installed. Automatically tracks insertions, deletions.
2) For mutations, one can get to the observer either through MF (or a forwarding call in the builder if that's convenient).

I'm not sure if there's ever a case where it makes sense to have different observers installed in the MF and the builders (that IMO will lead to confusions).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81899