GlobalISel: Do not set observer of MachineIRBuilder in LegalizerHelper

[arsenm]

Summary

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.

[arsenm]

ping

[arsenm]

Rebase

[aditya_nandakumar]

My thought on this Observer ownership is as follows (which I think I mentioned in another review).

• Each pass will setup an observer(wrapper) and install it to the MF. The MF at this point has the reference to the observer, and the pass owns each observer as it decides what it needs to observe and how.
• At the end of each pass, just reset the observer (there are RAII helpers for this).
• Remove the observer in MachineIRBuilder (is there a need for this?).
• No need to thread observer through APIs. Instead we get it through MF.getObserver().

Thoughts?

[arsenm]

In D86226#2250056, @aditya_nandakumar wrote:

My thought on this Observer ownership is as follows (which I think I mentioned in another review).

• Each pass will setup an observer(wrapper) and install it to the MF. The MF at this point has the reference to the observer, and the pass owns each observer as it decides what it needs to observe and how.
• At the end of each pass, just reset the observer (there are RAII helpers for this).
• Remove the observer in MachineIRBuilder (is there a need for this?).
• No need to thread observer through APIs. Instead we get it through MF.getObserver().

Thoughts?

I'm mostly concerned with the wonky way we set register banks on new instructions with an observer. This isn't pass / function level, and is on an individual instruction we're legalizing.

[aditya_nandakumar]

In D86226#2250210, @arsenm wrote:

In D86226#2250056, @aditya_nandakumar wrote:

My thought on this Observer ownership is as follows (which I think I mentioned in another review).

• Each pass will setup an observer(wrapper) and install it to the MF. The MF at this point has the reference to the observer, and the pass owns each observer as it decides what it needs to observe and how.
• At the end of each pass, just reset the observer (there are RAII helpers for this).
• Remove the observer in MachineIRBuilder (is there a need for this?).
• No need to thread observer through APIs. Instead we get it through MF.getObserver().

Thoughts?

I'm mostly concerned with the wonky way we set register banks on new instructions with an observer. This isn't pass / function level, and is on an individual instruction we're legalizing.

Sure, but what's the observer tracking in these cases (I admit we don't use the regbankselect pass in our out of tree backend and I may be missing the pain points here).

[arsenm]

In D86226#2250248, @aditya_nandakumar wrote:

In D86226#2250210, @arsenm wrote:

In D86226#2250056, @aditya_nandakumar wrote:

My thought on this Observer ownership is as follows (which I think I mentioned in another review).

• Each pass will setup an observer(wrapper) and install it to the MF. The MF at this point has the reference to the observer, and the pass owns each observer as it decides what it needs to observe and how.
• At the end of each pass, just reset the observer (there are RAII helpers for this).
• Remove the observer in MachineIRBuilder (is there a need for this?).
• No need to thread observer through APIs. Instead we get it through MF.getObserver().

Thoughts?

I'm mostly concerned with the wonky way we set register banks on new instructions with an observer. This isn't pass / function level, and is on an individual instruction we're legalizing.

Sure, but what's the observer tracking in these cases (I admit we don't use the regbankselect pass in our out of tree backend and I may be missing the pain points here).

It just grabs all the newly created / modified instructions, and then assigning register banks to the registers which haven't been set. This is manageable for the AMDGPU case where the individual opcodes don't really matter, but I would think other targets would struggle using it this way. I don't particularly like using the observer for this. It may be possible to have regbankselect iteratively work on the newly inserted instructions instead, but I haven't tried to do this.

[arsenm]

ping

[arsenm]

Rebase

[aemerson]

LGTM. Having looked into this recently we should definitely from the MIRBuilder's observer.

[arsenm]

d55d592a921f1cd6a922bff0000f6662f8722d9c

llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h

public:
   }
 
   /// Try to replace DstReg with SrcReg or build a COPY instruction
   /// depending on the register constraints.
   static void replaceRegOrBuildCopy(Register DstReg, Register SrcReg,
                                     MachineRegisterInfo &MRI,
                                     MachineIRBuilder &Builder,
                                     SmallVectorImpl<Register> &UpdatedDefs,
-                                    GISelObserverWrapper &Observer) {
+                                    GISelChangeObserver &Observer) {
     if (!llvm::canReplaceReg(DstReg, SrcReg, MRI)) {
       Builder.buildCopy(DstReg, SrcReg);
       UpdatedDefs.push_back(DstReg);
       return;
     }
     SmallVector<MachineInstr *, 4> UseMIs;
     // Get the users and notify the observer before replacing.
     for (auto &UseMI : MRI.use_instructions(DstReg)) {
static unsigned getDefIndex(const MachineInstr &MI, Register SearchDef) {
     }
 
     return DefIdx;
   }
 
   bool tryCombineUnmergeValues(MachineInstr &MI,
                                SmallVectorImpl<MachineInstr *> &DeadInsts,
                                SmallVectorImpl<Register> &UpdatedDefs,
-                               GISelObserverWrapper &Observer) {
+                               GISelChangeObserver &Observer) {
     assert(MI.getOpcode() == TargetOpcode::G_UNMERGE_VALUES);
 
     unsigned NumDefs = MI.getNumOperands() - 1;
     Register SrcReg = MI.getOperand(NumDefs).getReg();
     MachineInstr *SrcDef = getDefIgnoringCopies(SrcReg, MRI);
     if (!SrcDef)
       return false;
 

llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h

public:
   }
 
   MachineIRBuilder(MachineInstr &MI) :
     MachineIRBuilder(*MI.getParent(), MI.getIterator()) {
     setInstr(MI);
     setDebugLoc(MI.getDebugLoc());
   }
 
+  MachineIRBuilder(MachineInstr &MI, GISelChangeObserver &Observer) :
+    MachineIRBuilder(MI) {
+    setChangeObserver(Observer);
+  }
+
   virtual ~MachineIRBuilder() = default;
 
   MachineIRBuilder(const MachineIRBuilderState &BState) : State(BState) {}
 
   const TargetInstrInfo &getTII() {
     assert(State.TII && "TargetInstrInfo is not set");
     return *State.TII;
   }

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

static Type *getFloatTypeForLLT(LLVMContext &Ctx, LLT Ty) {
   }
 }
 
 LegalizerHelper::LegalizerHelper(MachineFunction &MF,
                                  GISelChangeObserver &Observer,
                                  MachineIRBuilder &Builder)
     : MIRBuilder(Builder), Observer(Observer), MRI(MF.getRegInfo()),
       LI(*MF.getSubtarget().getLegalizerInfo()),
-      TLI(*MF.getSubtarget().getTargetLowering()) {
-  MIRBuilder.setChangeObserver(Observer);
-}
+      TLI(*MF.getSubtarget().getTargetLowering()) { }
 
 LegalizerHelper::LegalizerHelper(MachineFunction &MF, const LegalizerInfo &LI,
                                  GISelChangeObserver &Observer,
                                  MachineIRBuilder &B)
   : MIRBuilder(B), Observer(Observer), MRI(MF.getRegInfo()), LI(LI),
-    TLI(*MF.getSubtarget().getTargetLowering()) {
-  MIRBuilder.setChangeObserver(Observer);
-}
+    TLI(*MF.getSubtarget().getTargetLowering()) { }
+
 LegalizerHelper::LegalizeResult
 LegalizerHelper::legalizeInstrStep(MachineInstr &MI) {
   LLVM_DEBUG(dbgs() << "Legalizing: " << MI);
 
   MIRBuilder.setInstrAndDebugLoc(MI);
 
   if (MI.getOpcode() == TargetOpcode::G_INTRINSIC ||
       MI.getOpcode() == TargetOpcode::G_INTRINSIC_W_SIDE_EFFECTS)
LegalizerHelper::LegalizeResult LegalizerHelper::lowerSelect(MachineInstr &MI) {
 
   auto NotMask = MIRBuilder.buildNot(MaskTy, MaskReg);
   auto NewOp1 = MIRBuilder.buildAnd(MaskTy, Op1Reg, MaskReg);
   auto NewOp2 = MIRBuilder.buildAnd(MaskTy, Op2Reg, NotMask);
   MIRBuilder.buildOr(DstReg, NewOp1, NewOp2);
   MI.eraseFromParent();
   return Legalized;
 }
 No newline at end of file

llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp

#endif
   void erasingInstr(MachineInstr &MI) override {}
 
   void createdInstr(MachineInstr &MI) override {
     // At this point, the instruction was just inserted and has no operands.
     NewInsts.push_back(&MI);
   }
 
   void changingInstr(MachineInstr &MI) override {}
-  void changedInstr(MachineInstr &MI) override {}
+  void changedInstr(MachineInstr &MI) override {
+    // FIXME: In principle we should probably add the instruction to NewInsts,
+    // but the way the LegalizerHelper uses the observer, we will always see the
+    // registers we need to set the regbank on also referenced in a new
+    // instruction.
+  }
 };
 
 }
 AMDGPURegisterBankInfo::AMDGPURegisterBankInfo(const GCNSubtarget &ST)
     : AMDGPUGenRegisterBankInfo(),
       Subtarget(ST),
       TRI(Subtarget.getRegisterInfo()),
       TII(Subtarget.getInstrInfo()) {
if (PtrBank == &AMDGPU::SGPRRegBank) {
     if (LoadSize != 96)
       return false;
 
     MachineMemOperand *MMO = *MI.memoperands_begin();
     Register PtrReg = MI.getOperand(1).getReg();
     // 96-bit loads are only available for vector loads. We need to split this
     // into a 64-bit part, and 32 (unless we can widen to a 128-bit load).
 
-    MachineIRBuilder B(MI);
     ApplyRegBankMapping O(*this, MRI, &AMDGPU::SGPRRegBank);
-    GISelObserverWrapper Observer(&O);
-    B.setChangeObserver(Observer);
+    MachineIRBuilder B(MI, O);
 
     if (MMO->getAlign() < Align(16)) {
       LLT Part64, Part32;
       std::tie(Part64, Part32) = splitUnequalType(LoadTy, 64);
       auto Load0 = B.buildLoadFromOffset(Part64, PtrReg, *MMO, 0);
       auto Load1 = B.buildLoadFromOffset(Part32, PtrReg, *MMO, 8);
 
       auto Undef = B.buildUndef(LoadTy);
bool AMDGPURegisterBankInfo::applyMappingLoad(MachineInstr &MI,
   assert(LoadSize % MaxNonSmrdLoadSize == 0);
 
   // RegBankSelect only emits scalar types, so we need to reset the pointer
   // operand to a pointer type.
   Register BasePtrReg = SrcRegs[0];
   LLT PtrTy = MRI.getType(MI.getOperand(1).getReg());
   MRI.setType(BasePtrReg, PtrTy);
 
-  MachineIRBuilder B(MI);
-
   unsigned NumSplitParts = LoadTy.getSizeInBits() / MaxNonSmrdLoadSize;
   const LLT LoadSplitTy = LoadTy.divide(NumSplitParts);
-  ApplyRegBankMapping O(*this, MRI, &AMDGPU::VGPRRegBank);
-  GISelObserverWrapper Observer(&O);
-  B.setChangeObserver(Observer);
+  ApplyRegBankMapping Observer(*this, MRI, &AMDGPU::VGPRRegBank);
+  MachineIRBuilder B(MI, Observer);
   LegalizerHelper Helper(B.getMF(), Observer, B);
 
   if (LoadTy.isVector()) {
     if (Helper.fewerElementsVector(MI, 0, LoadSplitTy) != LegalizerHelper::Legalized)
       return false;
   } else {
     if (Helper.narrowScalar(MI, 0, LoadSplitTy) != LegalizerHelper::Legalized)
       return false;
if (SizeBank != &AMDGPU::SGPRRegBank)
     return false;
 
   LLT PtrTy = MRI.getType(Dst);
   LLT IntPtrTy = LLT::scalar(PtrTy.getSizeInBits());
 
   const SIMachineFunctionInfo *Info = MF.getInfo<SIMachineFunctionInfo>();
   Register SPReg = Info->getStackPtrOffsetReg();
   ApplyRegBankMapping ApplyBank(*this, MRI, &AMDGPU::SGPRRegBank);
-  GISelObserverWrapper Observer(&ApplyBank);
-
-  MachineIRBuilder B(MI);
-  B.setChangeObserver(Observer);
+  MachineIRBuilder B(MI, ApplyBank);
 
   auto WaveSize = B.buildConstant(LLT::scalar(32), ST.getWavefrontSizeLog2());
   auto ScaledSize = B.buildShl(IntPtrTy, AllocSize, WaveSize);
 
   auto SPCopy = B.buildCopy(PtrTy, SPReg);
   if (Alignment > TFI.getStackAlign()) {
     auto PtrAdd = B.buildPtrAdd(PtrTy, SPCopy, ScaledSize);
     B.buildMaskLowPtrBits(Dst, PtrAdd,
bool AMDGPURegisterBankInfo::applyMappingBFEIntrinsic(
 
   Register SrcReg = MI.getOperand(2).getReg();
   Register OffsetReg = MI.getOperand(3).getReg();
   Register WidthReg = MI.getOperand(4).getReg();
 
   // The scalar form packs the offset and width in a single operand.
 
   ApplyRegBankMapping ApplyBank(*this, MRI, &AMDGPU::SGPRRegBank);
-  GISelObserverWrapper Observer(&ApplyBank);
-  MachineIRBuilder B(MI);
-  B.setChangeObserver(Observer);
+  MachineIRBuilder B(MI, ApplyBank);
 
   // Ensure the high bits are clear to insert the offset.
   auto OffsetMask = B.buildConstant(S32, maskTrailingOnes<unsigned>(6));
   auto ClampOffset = B.buildAnd(S32, OffsetReg, OffsetMask);
 
   // Zeros out the low bits, so don't bother clamping the input value.
   auto ShiftWidth = B.buildShl(S32, WidthReg, B.buildConstant(S32, 16));
 
case AMDGPU::G_PHI: {
     }
 
     // Phi handling is strange and only considers the bank of the destination.
     substituteSimpleCopyRegs(OpdMapper, 0);
 
     // Promote SGPR/VGPR booleans to s32
     MachineFunction *MF = MI.getParent()->getParent();
     ApplyRegBankMapping ApplyBank(*this, MRI, DstBank);
-    GISelObserverWrapper Observer(&ApplyBank);
-    MachineIRBuilder B(MI);
-    LegalizerHelper Helper(*MF, Observer, B);
+    MachineIRBuilder B(MI, ApplyBank);
+    LegalizerHelper Helper(*MF, ApplyBank, B);
 
     if (Helper.widenScalar(MI, 0, S32) != LegalizerHelper::Legalized)
       llvm_unreachable("widen scalar should have succeeded");
 
     return;
   }
   case AMDGPU::G_ICMP:
   case AMDGPU::G_UADDO:
case AMDGPU::G_XOR: {
     if (DstTy.getSizeInBits() == 1) {
       const RegisterBank *DstBank =
         OpdMapper.getInstrMapping().getOperandMapping(0).BreakDown[0].RegBank;
       if (DstBank == &AMDGPU::VCCRegBank)
         break;
 
       MachineFunction *MF = MI.getParent()->getParent();
       ApplyRegBankMapping ApplyBank(*this, MRI, DstBank);
-      GISelObserverWrapper Observer(&ApplyBank);
-      MachineIRBuilder B(MI);
-      LegalizerHelper Helper(*MF, Observer, B);
+      MachineIRBuilder B(MI, ApplyBank);
+      LegalizerHelper Helper(*MF, ApplyBank, B);
 
       if (Helper.widenScalar(MI, 0, LLT::scalar(32)) !=
           LegalizerHelper::Legalized)
         llvm_unreachable("widen scalar should have succeeded");
       return;
     }
 
     if (DstTy.getSizeInBits() != 64)
case AMDGPU::G_ASHR: {
     const RegisterBank *DstBank =
       OpdMapper.getInstrMapping().getOperandMapping(0).BreakDown[0].RegBank;
     if (DstBank == &AMDGPU::VGPRRegBank)
       break;
 
     const LLT S32 = LLT::scalar(32);
     MachineBasicBlock *MBB = MI.getParent();
     MachineFunction *MF = MBB->getParent();
-    MachineIRBuilder B(MI);
     ApplyRegBankMapping ApplySALU(*this, MRI, &AMDGPU::SGPRRegBank);
-    GISelObserverWrapper Observer(&ApplySALU);
+    MachineIRBuilder B(MI, ApplySALU);
 
     if (DstTy.isVector()) {
-      B.setChangeObserver(Observer);
-
       Register WideSrc0Lo, WideSrc0Hi;
       Register WideSrc1Lo, WideSrc1Hi;
 
       std::tie(WideSrc0Lo, WideSrc0Hi)
         = unpackV2S16ToS32(B, MI.getOperand(1).getReg(), AMDGPU::G_ANYEXT);
       std::tie(WideSrc1Lo, WideSrc1Hi)
         = unpackV2S16ToS32(B, MI.getOperand(2).getReg(), AMDGPU::G_ANYEXT);
       auto Lo = B.buildInstr(MI.getOpcode(), {S32}, {WideSrc0Lo, WideSrc1Lo});
       auto Hi = B.buildInstr(MI.getOpcode(), {S32}, {WideSrc0Hi, WideSrc1Hi});
       B.buildBuildVectorTrunc(DstReg, {Lo.getReg(0), Hi.getReg(0)});
       MI.eraseFromParent();
     } else {
-      LegalizerHelper Helper(*MF, Observer, B);
+      LegalizerHelper Helper(*MF, ApplySALU, B);
 
       if (Helper.widenScalar(MI, 0, S32) != LegalizerHelper::Legalized)
         llvm_unreachable("widen scalar should have succeeded");
 
       // FIXME: s16 shift amounts should be legal.
       if (Opc == AMDGPU::G_SHL || Opc == AMDGPU::G_LSHR ||
           Opc == AMDGPU::G_ASHR) {
         B.setInsertPt(*MBB, MI.getIterator());
case AMDGPU::G_UMAX: {
     // Turn scalar min/max into a compare and select.
     LLT Ty = MRI.getType(DstReg);
     const LLT S32 = LLT::scalar(32);
     const LLT S16 = LLT::scalar(16);
     const LLT V2S16 = LLT::vector(2, 16);
 
     if (Ty == V2S16) {
       ApplyRegBankMapping ApplySALU(*this, MRI, &AMDGPU::SGPRRegBank);
-      GISelObserverWrapper Observer(&ApplySALU);
-      B.setChangeObserver(Observer);
+      B.setChangeObserver(ApplySALU);
 
       // Need to widen to s32, and expand as cmp + select, and avoid producing
       // illegal vector extends or unmerges that would need further
       // legalization.
       //
       // TODO: Should we just readfirstlane? That should probably be handled
       // with a UniformVGPR register bank that wouldn't need special
       // consideration here.
if (Ty == V2S16) {
       const CmpInst::Predicate Pred = minMaxToCompare(MI.getOpcode());
       buildExpandedScalarMinMax(B, Pred, Lo, WideSrc0Lo, WideSrc1Lo);
       buildExpandedScalarMinMax(B, Pred, Hi, WideSrc0Hi, WideSrc1Hi);
 
       B.buildBuildVectorTrunc(Dst, {Lo, Hi});
       MI.eraseFromParent();
     } else if (Ty == S16) {
       ApplyRegBankMapping ApplySALU(*this, MRI, &AMDGPU::SGPRRegBank);
-      GISelObserverWrapper Observer(&ApplySALU);
-      LegalizerHelper Helper(*MF, Observer, B);
+      B.setChangeObserver(ApplySALU);
+      LegalizerHelper Helper(*MF, ApplySALU, B);
 
       // Need to widen to s32, and expand as cmp + select.
       if (Helper.widenScalar(MI, 0, S32) != LegalizerHelper::Legalized)
         llvm_unreachable("widenScalar should have succeeded");
 
       // FIXME: This is relying on widenScalar leaving MI in place.
       lowerScalarMinMax(B, MI);
     } else
case AMDGPU::G_SEXT_INREG: {
     Register DstReg = MI.getOperand(0).getReg();
     MRI.setRegBank(DstReg, AMDGPU::VGPRRegBank);
     MI.eraseFromParent();
     return;
   }
   case AMDGPU::G_CTPOP:
   case AMDGPU::G_CTLZ_ZERO_UNDEF:
   case AMDGPU::G_CTTZ_ZERO_UNDEF: {
-    MachineIRBuilder B(MI);
-    MachineFunction &MF = B.getMF();
-
     const RegisterBank *DstBank =
       OpdMapper.getInstrMapping().getOperandMapping(0).BreakDown[0].RegBank;
     if (DstBank == &AMDGPU::SGPRRegBank)
       break;
 
     Register SrcReg = MI.getOperand(1).getReg();
     const LLT S32 = LLT::scalar(32);
     LLT Ty = MRI.getType(SrcReg);
     if (Ty == S32)
       break;
 
     ApplyRegBankMapping ApplyVALU(*this, MRI, &AMDGPU::VGPRRegBank);
-    GISelObserverWrapper Observer(&ApplyVALU);
-    LegalizerHelper Helper(MF, Observer, B);
+    MachineIRBuilder B(MI, ApplyVALU);
+
+    MachineFunction &MF = B.getMF();
+    LegalizerHelper Helper(MF, ApplyVALU, B);
 
     if (Helper.narrowScalar(MI, 1, S32) != LegalizerHelper::Legalized)
       llvm_unreachable("narrowScalar should have succeeded");
     return;
   }
   case AMDGPU::G_SEXT:
   case AMDGPU::G_ZEXT:
   case AMDGPU::G_ANYEXT: {

llvm/lib/Target/Mips/MipsRegisterBankInfo.cpp

void MipsRegisterBankInfo::setRegBank(MachineInstr &MI,
   }
   default:
     llvm_unreachable("Unexpected opcode.");
   }
 }
 
 static void
 combineAwayG_UNMERGE_VALUES(LegalizationArtifactCombiner &ArtCombiner,
-                            MachineInstr &MI, GISelObserverWrapper &Observer) {
+                            MachineInstr &MI, GISelChangeObserver &Observer) {
   SmallVector<Register, 4> UpdatedDefs;
   SmallVector<MachineInstr *, 2> DeadInstrs;
   ArtCombiner.tryCombineUnmergeValues(MI, DeadInstrs, UpdatedDefs, Observer);
   for (MachineInstr *DeadMI : DeadInstrs)
     DeadMI->eraseFromParent();
 }
 
 void MipsRegisterBankInfo::applyMappingImpl(
     const OperandsMapper &OpdMapper) const {
   MachineInstr &MI = OpdMapper.getMI();
   InstListTy NewInstrs;
-  MachineIRBuilder B(MI);
   MachineFunction *MF = MI.getMF();
   MachineRegisterInfo &MRI = OpdMapper.getMRI();
   const LegalizerInfo &LegInfo = *MF->getSubtarget().getLegalizerInfo();
 
   InstManager NewInstrObserver(NewInstrs);
-  GISelObserverWrapper WrapperObserver(&NewInstrObserver);
-  LegalizerHelper Helper(*MF, WrapperObserver, B);
+  MachineIRBuilder B(MI, NewInstrObserver);
+  LegalizerHelper Helper(*MF, NewInstrObserver, B);
   LegalizationArtifactCombiner ArtCombiner(B, MF->getRegInfo(), LegInfo);
 
   switch (MI.getOpcode()) {
   case TargetOpcode::G_LOAD:
   case TargetOpcode::G_STORE:
   case TargetOpcode::G_PHI:
   case TargetOpcode::G_SELECT:
   case TargetOpcode::G_IMPLICIT_DEF: {
     Helper.narrowScalar(MI, 0, LLT::scalar(32));
     // Handle new instructions.
     while (!NewInstrs.empty()) {
       MachineInstr *NewMI = NewInstrs.pop_back_val();
       // This is new G_UNMERGE that was created during narrowScalar and will
       // not be considered for regbank selection. RegBankSelect for mips
       // visits/makes corresponding G_MERGE first. Combine them here.
       if (NewMI->getOpcode() == TargetOpcode::G_UNMERGE_VALUES)
-        combineAwayG_UNMERGE_VALUES(ArtCombiner, *NewMI, WrapperObserver);
+        combineAwayG_UNMERGE_VALUES(ArtCombiner, *NewMI, NewInstrObserver);
       // This G_MERGE will be combined away when its corresponding G_UNMERGE
       // gets regBankSelected.
       else if (NewMI->getOpcode() == TargetOpcode::G_MERGE_VALUES)
         continue;
       else
         // Manually set register banks for def operands to 32 bit gprb.
         setRegBank(*NewMI, MRI);
     }
     return;
   }
   case TargetOpcode::G_UNMERGE_VALUES:
-    combineAwayG_UNMERGE_VALUES(ArtCombiner, MI, WrapperObserver);
+    combineAwayG_UNMERGE_VALUES(ArtCombiner, MI, NewInstrObserver);
     return;
   default:
     break;
   }
 
   return applyDefaultMapping(OpdMapper);
 }

llvm/test/CodeGen/AArch64/GlobalISel/legalize-ext-csedebug-output.mir

 # RUN: llc -march=aarch64 -run-pass=legalizer %s -o - -enable-cse-in-legalizer=1 -debug-only=cseinfo 2>&1 | FileCheck %s
 # Test the -debug-only=cseinfo option here to make sure we're recording new instructions and printing the hit count
 # at the end.
 # REQUIRES: asserts
 ---
 name:            test_cse_in_legalizer
 body:             |
   bb.0.entry:
     ; CHECK: CSEInfo::Add MI: %{{[0-9]+}}:_(s8) = G_TRUNC
     ; CHECK: CSEInfo::Add MI: %{{[0-9]+}}:_(s32) = G_ZEXT
     ; CHECK: CSEInfo::Add MI: %{{[0-9]+}}:_(s8) = G_TRUNC
     ; CHECK: CSEInfo::Add MI: %{{[0-9]+}}:_(s32) = G_ZEXT
     ; CHECK: CSEInfo::Recording new MI G_CONSTANT
-    ; CHECK: CSEInfo::Recording new MI G_CONSTANT
-    ; CHECK: CSEInfo::Recording new MI G_TRUNC
     ; CHECK: CSEInfo::Recording new MI G_TRUNC
     ; CHECK: CSEInfo::Recording new MI G_AND
-    ; CHECK: CSEInfo::Recording new MI G_AND
     ; CHECK: CSEInfo::Found Instr %{{[0-9]+}}:_(s32) = G_CONSTANT
     ; CHECK: CSEInfo::Found Instr %{{[0-9]+}}:_(s32) = G_TRUNC
     ; CHECK: CSEInfo::Found Instr %{{[0-9]+}}:_(s32) = G_AND
     ; CHECK: CSEInfo::CSE Hit for Opc {{[0-9]+}} : 1
     ; CHECK: CSEInfo::CSE Hit for Opc {{[0-9]+}} : 1
     ; CHECK: CSEInfo::CSE Hit for Opc {{[0-9]+}} : 1
     %0:_(s64) = COPY $x0
     %1:_(s8) = G_TRUNC %0(s64)
     %19:_(s32) = G_ZEXT %1(s8)
     $w0 = COPY %19(s32)
     %2:_(s8) = G_TRUNC %0(s64)
     %20:_(s32) = G_ZEXT %2(s8)
     $w0 = COPY %20(s32)