[GISel] `applyExtendThroughPhis` is unsound for ops with multiple destination operands

[widlarizer]

Discovered in a proprietary fork, so no reproducer.

before:

%91:_(s32), %92:_(s32) = G_UNMERGE_VALUES %8:_(s64) ; g_105.f0, g_105.f1,pad
...
%75:_(s32) = G_PHI %92:_(s32), %bb.4, %81:_(s32), %bb.3, %95:_(s32), %bb.2 ; g_105.f1,pad

combiner says:

Try combining %75:_(s32) = G_PHI %92:_(s32), %bb.4, %81:_(s32), %bb.3, %95:_(s32), %bb.2
Applying rule 'extend_through_phis'
Creating: G_ANYEXT

CSEInfo::Recording new MI G_ANYEXT
Creating: G_ANYEXT

CSEInfo::Recording new MI G_ANYEXT
Creating: G_ANYEXT

CSEInfo::Recording new MI G_ANYEXT
Creating: %103:_(s64) = G_PHI %120:_(s64), %bb.4, %121:_(s64), %bb.3, %122:_(s64), %bb.2

Erasing: %103:_(s64) = G_ANYEXT %75:_(s32)

Created: %120:_(s64) = G_ANYEXT %91:_(s32)
Created: %121:_(s64) = G_ANYEXT %81:_(s32)
Created: %122:_(s64) = G_ANYEXT %95:_(s32)
Created: %103:_(s64) = G_PHI %120:_(s64), %bb.4, %121:_(s64), %bb.3, %122:_(s64), %bb.2

what I guess it looks like after this one combiner rule:

%120:_(s64) = G_ANYEXT %91:_(s32)
%103:_(s64) = G_PHI %120:_(s64), %bb.4, %121:_(s64), %bb.3, %122:_(s64), %bb.2

after all of post legalizer combiner:

%8:_(s64) = G_LOAD %6:_(p0) :: (load (s64) from %ir.1, align 4)
%103:_(s64) = G_PHI %8:_(s64), %bb.4, %121:_(s64), %bb.3, %122:_(s64), %bb.2   ; g_105.f0,f1,pad

You can see that a use of %92 was incorrectly replaced with an anyext use of %91. You can turn this into a crash, "bug scanner"-style, like this (diff line nos likely wrong):

--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -4024,6 +4024,7 @@ void CombinerHelper::applyExtendThroughPhis(MachineInstr &MI,
     Builder.setDebugLoc(MI.getDebugLoc());
     auto NewExt = Builder.buildExtOrTrunc(ExtMI->getOpcode(), ExtTy,
                                           SrcMI->getOperand(0).getReg());
+    assert(MI.getOperand(SrcIdx).getReg() == SrcMI->getOperand(0).getReg());
     OldToNewSrcMap[SrcMI] = NewExt;
   }

PR incoming.

Tagging @aemerson as author of combiner rule extend_through_phis, @andcarminati @agostonrobert as collaborators

[widlarizer]

A simpler explanation may be in order as well. The purpose of the problematic combiner rule is to propagate an extension up past a phi. Consider the following situation:

%1, %2 = G_FOO ...
%3 = G_PHI %2, ...
%4 = G_ANYEXT %3

Currently, the rule looks will transform it into something like this:

%1, %2 = G_FOO ...
%3 = G_ANYEXT %1
%4 = G_PHI %3, ...

But it should transform it into something like this:

%1, %2 = G_FOO ...
%3 = G_ANYEXT %2
%4 = G_PHI %3, ...

This is because it makes the incorrect assumption that there is one unique destination operand at index 0. The rule implementation already knows which reg to extend, because the rule is applied to the phi, and the reg is an argument of the phi. We shouldn't ask the source MI about its operand 0, and instead should know existing knowledge of which register we're interested in.

[andcarminati]

This sounds like a real problem to me. If a G_PHI input comes from a G_UNMERGE_VALUES it can be a situation where operand 0 may be incorrect to G_ANYEXT.

[aemerson]

Ah right, multiple destination registers. I'm guessing it may not be possible to have a test for this because of the target hook for profitability. If so I'm ok with committing this as is.

[widlarizer]

There's an early return for anyext actually, so as long as we're moving anyext, it will happen regardless of the hook
return true; // G_ANYEXT is usually free.