[PATCH] D86226: GlobalISel: Do not set observer of MachineIRBuilder in LegalizerHelper

[Matt Arsenault via Phabricator via llvm-commits]

arsenm created this revision.
arsenm added reviewers: aditya_nandakumar, dsanders, paquette, aemerson.
Herald added subscribers: kerbowa, atanasyan, hiraditya, arichardson, tpr, rovka, nhaehnle, jvesely, sdardis.
Herald added a project: LLVM.
arsenm requested review of this revision.
Herald added a subscriber: wdng.

This fixes double printing of insertion debug messages in the
legalizer.

      

Try to cleanup usage of observers. Currently the use of observers is
pretty hard to follow and it's not clear what is responsible for
them. Observers are referenced in 3 places:

      

1. In the MachineFunction
2. In the MachineIRBuilder
3. In the LegalizerHelper

The observers in the MachineFunction and MachineIRBuilder are both
called only on insertions, and are redundant with each other. The
source of the double printing was the same observer was added to both
the MachineFunction, and the MachineIRBuilder. One of these references
needs to be removed. Arguably observers in general should be fully
removed from one or the other, but it may be useful to have a local
observer in the MachineIRBuilder that is not added to the function's
observers. Alternatively, the wrapper observer could manage a local
observer in one place.

      

The LegalizerHelper only ever calls the observer on changing/changed
instructions, and never insertions. Logically these are two different
types of observers, for changes and for insertions.

      

Additionally, some places used the GISelObserverWrapper when they only
needed a single observer they could use directly.

      

Setting the observer in the LegalizerHelper constructor is not
flexible enough if the LegalizerHelper is constructed anywhere outside
the one used by the legalizer. AMDGPU calls the LegalizerHelper in
RegBankSelect, and needs to use a local observer to apply the regbank
to newly created instructions. Currently it accomplishes this by
constructing a local MachineIRBuilder. I'm trying to move the
MachineIRBuilder to be owned/maintained by the RegBankSelect pass
itself, but the locally constructed LegalizerHelper would reset the
observer.

      

Mips also has a special case use of the LegalizationArtifactCombiner
in applyMappingImpl; I think we do need to run the artifact combiner
during RegBankSelect, but in a more consistent way outside of
applyMappingImpl.


https://reviews.llvm.org/D86226

Files:
  llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
  llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
  llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
  llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp
  llvm/lib/Target/Mips/MipsRegisterBankInfo.cpp
  llvm/test/CodeGen/AArch64/GlobalISel/legalize-ext-csedebug-output.mir

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D86226.286585.patch
Type: text/x-patch
Size: 12185 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200819/9d42a06e/attachment.bin>