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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.

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


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

https://reviews.llvm.org/D81461