[gicombiner] Unify common state for current targets into CommonTargetCombinerHelperState
Needs ReviewPublic
Actions

Authored by dsanders on Jun 15 2020, 5:31 PM.

Details

Reviewers

Summary

At this point tryCombine*() in the existing combiners takes a single MI argument.

Depends on D81889

llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
278–279	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?

dsanders marked an inline comment as done.Jun 16 2020, 10:31 AM

dsanders added inline comments.

llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
278–279	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

llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
278–279	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, MF always has observer installed. Automatically tracks insertions, deletions. 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).

llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
278–279	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, MF always has observer installed. Automatically tracks insertions, deletions. 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). +1

llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
278–279	So in this case, the Observer here is supposed to also be present in either/or the MIRBuilder or MachineFunction, but the combiner also needs to inform this? Should there be an assert this observer is present in the builder/function?

llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
278–279	I was thinking that the MF will have the observer, and the MIRBuilder does not store the observer at all (and calls to Builder.getObserver() does the equivalent of Builder.getMF().getObserver()). With this, we can stop passing in the observer to all APIs.