[PATCH] D81461: GlobalISel: Fix double printing new instructions in legalizer

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar added a comment.

In D81461#2082939 <https://reviews.llvm.org/D81461#2082939>, @arsenm wrote:

> In D81461#2082923 <https://reviews.llvm.org/D81461#2082923>, @arsenm wrote:
>
> > In D81461#2082557 <https://reviews.llvm.org/D81461#2082557>, @aditya_nandakumar wrote:
> >
> > > In D81461#2082149 <https://reviews.llvm.org/D81461#2082149>, @arsenm wrote:
> > >
> > > > It seems both the MachineFunction and MachineIRBuilder have insertion observers; this seems redundant. I think the machine function observers should be removed?
> > >
> > >
> > > MachineFunction observers are useful even if instructions are built without MIRBuilders (say using BuildMI) so CSE can be aware of it. Similar for deletions. To me, since the MF observer is always called, the one in MachineIRBuilder seems redundant.
> >
> >
> > I don't think I would expect CSE or any magic when using BuildMI or generally touching the function. I think anything before selection should be going through MachineIRBuilder?
>
>
> I also don't see how CSE would work with BuildMI, since you don't see the full set of operands at construction as CSE requires. Even with MachineIRBuilder, you only get CSE if you construct the instruction with all of its operands initially. Trying to handle this for target instructions also generally starts to break down with implicit operands etc.




In D81461#2082939 <https://reviews.llvm.org/D81461#2082939>, @arsenm wrote:

> In D81461#2082923 <https://reviews.llvm.org/D81461#2082923>, @arsenm wrote:
>
> > In D81461#2082557 <https://reviews.llvm.org/D81461#2082557>, @aditya_nandakumar wrote:
> >
> > > In D81461#2082149 <https://reviews.llvm.org/D81461#2082149>, @arsenm wrote:
> > >
> > > > It seems both the MachineFunction and MachineIRBuilder have insertion observers; this seems redundant. I think the machine function observers should be removed?
> > >
> > >
> > > MachineFunction observers are useful even if instructions are built without MIRBuilders (say using BuildMI) so CSE can be aware of it. Similar for deletions. To me, since the MF observer is always called, the one in MachineIRBuilder seems redundant.
> >
> >
> > I don't think I would expect CSE or any magic when using BuildMI or generally touching the function. I think anything before selection should be going through MachineIRBuilder?
>
>
> I also don't see how CSE would work with BuildMI, since you don't see the full set of operands at construction as CSE requires. Even with MachineIRBuilder, you only get CSE if you construct the instruction with all of its operands initially. Trying to handle this for target instructions also generally starts to break down with implicit operands etc.


CSE needs to be aware of instructions that get deleted so it doesn't hold on to dangling references. That's the main use for the MF installed observer. The additional side effect (not really a feature that should be used) is that one can BuildMI an instruction. Later on if they try to build with the CSEMIRBuilder, they'd get the CSEd instruction.
This works because CSE lazily hashes instructions (it just keeps references to them and hashes them only during the next build call). Assumption is in most cases users completely build one instruction (even if by parts) and by the next call to build*, the previous instruction is fully built.

  CSEMIRBuilder CSEB;...
  // If the MF observer is set
  auto MIB1 = BuildMI(... G_ADD).addUse(x).addUse(y); // Or with regular builder MIRBuilder.buildAdd(s32, x, y); // Or MIRBuilder.buildInstr(G_ADD).addDef(Reg).addUse(x).addUse(y);
  auto MIB2 = CSEB.buildAdd(s32, x, y);
  MIB1.getInstr() == MIB2.getInstr();

Or this unit test

  --- a/llvm/unittests/CodeGen/GlobalISel/CSETest.cpp
  +++ b/llvm/unittests/CodeGen/GlobalISel/CSETest.cpp
  @@ -77,6 +77,17 @@ TEST_F(AArch64GISelMITest, TestCSE) {
     auto Undef0 = CSEB.buildUndef(s32);
     auto Undef1 = CSEB.buildUndef(s32);
     EXPECT_EQ(&*Undef0, &*Undef1);
  +
  +  GISelObserverWrapper WrapperObserver(&CSEInfo);
  +  RAIIMFObsDelInstaller Installer(*MF, WrapperObserver);
  +  MachineIRBuilder RegularBuilder(*MF);
  +  RegularBuilder.setInsertPt(*EntryMBB, EntryMBB->begin());
  +  auto NonCSEFMul = RegularBuilder.buildInstr(TargetOpcode::G_FMUL)
  +    .addDef(MRI->createGenericVirtualRegister(s32))
  +    .addUse(Copies[0])
  +    .addUse(Copies[1]);
  +  auto CSEFMul = CSEB.buildInstr(TargetOpcode::G_FMUL, {s32}, {Copies[0], Copies[1]});
  +  EXPECT_EQ(&*CSEFMul, &*NonCSEFMul);
   }


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

https://reviews.llvm.org/D81461