GlobalISel: Fix double printing new instructions in legalizer

[arsenm]

Summary

New instructions were getting printed both in createdInstr, and in the
final printNewInstrs, so it made it look like the same instructions
were created twice. This overall made reading the debug output
harder. Stop printing the initial construction and only print new
instructions in the summary at the end. This avoids printing the less
useful case where instructions are sometimes initially created with no
operands.

I'm not sure this is the correct instance to remove; now the visible
ordering is different. Now you will typically see the one erased
instruction message before all the new instructions in order. I think
this is the more logical view of typical legalization changes,
although it's mechanically backwards from the normal
insert-new-erase-old pattern.

[arsenm]

Actually I think this isn't the main source of double printing. The same observer seems to get called twice:

First through this:
#0 llvm::GISelObserverWrapper::createdInstr (this=0x7fffffffb658, MI=...)
#1 0x0000000003b9ca51 in llvm::GISelObserverWrapper::MF_HandleInsertion (this=0x7fffffffb658, MI=...)
#2 0x00000000055f703e in llvm::MachineFunction::handleInsertion (this=0x792f510, MI=...)

And then later this:
#0 (anonymous namespace)::LegalizerWorkListManager::createdInstr (this=0x7fffffffb6e0, MI=...)
#1 0x0000000003b9cb88 in llvm::GISelObserverWrapper::createdInstr (this=0x7fffffffb658, MI=...)
#2 0x0000000003b9ca51 in llvm::GISelObserverWrapper::MF_HandleInsertion (this=0x7fffffffb658, MI=...)

[arsenm]

It seems both the MachineFunction and MachineIRBuilder have insertion observers; this seems redundant. I think the machine function observers should be removed?

[aditya_nandakumar]

In 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.

[dsanders]

LGTM

I'm not sure this is the correct instance to remove; now the visible
ordering is different. Now you will typically see the one erased
instruction message before all the new instructions in order. I think
this is the more logical view of typical legalization changes,
although it's mechanically backwards from the normal
insert-new-erase-old pattern.

Another reason for keeping the one in printNewInstrs() is that createInstr() sees the MI before its populated with operands. I think it's more useful to keep the one that's the fully built instruction

Actually I think this isn't the main source of double printing. The same observer seems to get called twice:
...

Was that with the same MI pointer? I assume it is but the dump doesn't make it clear

In D81461#2082557, @aditya_nandakumar wrote:

In 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'd lean towards the MachineFunction ones too although admittedly I haven't dug into the details there

[aditya_nandakumar]

In D81461#2082624, @dsanders wrote:

LGTM

I'm not sure this is the correct instance to remove; now the visible
ordering is different. Now you will typically see the one erased
instruction message before all the new instructions in order. I think
this is the more logical view of typical legalization changes,
although it's mechanically backwards from the normal
insert-new-erase-old pattern.

Another reason for keeping the one in printNewInstrs() is that createInstr() sees the MI before its populated with operands. I think it's more useful to keep the one that's the fully built instruction

Actually I think this isn't the main source of double printing. The same observer seems to get called twice:
...

Was that with the same MI pointer? I assume it is but the dump doesn't make it clear

I think this is with the same MI pointer.

In D81461#2082557, @aditya_nandakumar wrote:

In 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'd lean towards the MachineFunction ones too although admittedly I haven't dug into the details there

[arsenm]

In D81461#2082557, @aditya_nandakumar wrote:

In 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?

[arsenm]

In D81461#2082923, @arsenm wrote:

In D81461#2082557, @aditya_nandakumar wrote:

In 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.

[aditya_nandakumar]

In D81461#2082939, @arsenm wrote:

In D81461#2082923, @arsenm wrote:

In D81461#2082557, @aditya_nandakumar wrote:

In 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, @arsenm wrote:

In D81461#2082923, @arsenm wrote:

In D81461#2082557, @aditya_nandakumar wrote:

In 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);
 }

[arsenm]

6eeac6ae33046f022f2d2c857ef38d2329acfc88

[aditya_nandakumar]

Is there any interest in me checking in the above unit test? I initially didn't check it in as I thought this was more of a side effect of how things are implemented and not really how the APIs should be used.

[arsenm]

In D81461#2086302, @aditya_nandakumar wrote:

Is there any interest in me checking in the above unit test? I initially didn't check it in as I thought this was more of a side effect of how things are implemented and not really how the APIs should be used.

I think tests that show the expected behavior are useful even if you aren't supposed to use it

llvm/lib/CodeGen/GlobalISel/Legalizer.cpp

if (isPreISelGenericOpcode(MI.getOpcode())) {
       if (isArtifact(MI))
         ArtifactList.insert(&MI);
       else
         InstList.insert(&MI);
     }
   }
 
   void createdInstr(MachineInstr &MI) override {
-    LLVM_DEBUG(dbgs() << ".. .. New MI: " << MI);
     LLVM_DEBUG(NewMIs.push_back(&MI));
     createdOrChangedInstr(MI);
   }
 
   void printNewInstrs() {
     LLVM_DEBUG({
       for (const auto *MI : NewMIs)
         dbgs() << ".. .. New MI: " << *MI;