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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders marked an inline comment as done.
dsanders added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h:278-279
+protected:
+  GISelChangeObserver &Observer;
+  MachineIRBuilder &B;
+  CombinerHelper &Helper;
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81899