Q. about GlobalISel and order of instructions

Hello Global ISel fans,

I am trying to patch up some unittests that accidentally weren't
actually running (https://reviews.llvm.org/D95257). A number of
these look like all the right instructions are coming out, but
they aren't "in the right order." That is, I see some tests
expect something along the lines of

    %2:_(s16) = G_TRUNC
    %3:_(s10) = G_TRUNC %2:_(s16)
    %4:_(s10) = G_SEXT_INREG %3:_(s10)
    %5:_(s16) = G_SEXT %4:_(s10)

That is, each instruction feeds nicely into the next one.
But what I'm actually seeing is reordered:

    %5:_(s16) = G_SEXT %4:_(s10)
    %2:_(s16) = G_TRUNC
    %4:_(s10) = G_SEXT_INREG %3:_(s10)
    %3:_(s10) = G_TRUNC %2:_(s16)

Now, I can certainly patch up the test to match the order I see,
but... Is that actually correct? Naively it looks like we have
uses preceding defs, which just looks weird.

If the order I'm seeing is wrong (and it's just a standard dump
of a MachineFunction) then can somebody fix it? It will sure
make a lot less churn in the unittest.

Thanks,
--paulr

Hi Paul,

Thanks for the heads-up!

Hello Global ISel fans,

I am trying to patch up some unittests that accidentally weren't
actually running (https://reviews.llvm.org/D95257). A number of
these look like all the right instructions are coming out, but
they aren't "in the right order." That is, I see some tests
expect something along the lines of

   %2:_(s16) = G_TRUNC
   %3:_(s10) = G_TRUNC %2:_(s16)
   %4:_(s10) = G_SEXT_INREG %3:_(s10)
   %5:_(s16) = G_SEXT %4:_(s10)

That is, each instruction feeds nicely into the next one.
But what I'm actually seeing is reordered:

   %5:_(s16) = G_SEXT %4:_(s10)
   %2:_(s16) = G_TRUNC
   %4:_(s10) = G_SEXT_INREG %3:_(s10)
   %3:_(s10) = G_TRUNC %2:_(s16)

Now, I can certainly patch up the test to match the order I see,
but... Is that actually correct? Naively it looks like we have
uses preceding defs, which just looks weird.

That’s definitely broken!

If the order I'm seeing is wrong (and it's just a standard dump
of a MachineFunction) then can somebody fix it?

Are these the tests you marked as FIXME in the PR you’ve linked?

Cheers,
-Quentin

From: Quentin Colombet <qcolombet@apple.com>
Sent: Friday, January 29, 2021 1:32 PM
To: Robinson, Paul <paul.robinson@sony.com>
Cc: llvm-dev@lists.llvm.org
Subject: Re: [llvm-dev] Q. about GlobalISel and order of instructions

Hi Paul,

Thanks for the heads-up!

>
> Hello Global ISel fans,
>
> I am trying to patch up some unittests that accidentally weren't
> actually running
(https://urldefense.com/v3/https://reviews.llvm.org/D95257;!!JmoZiZGBv
3RvKRSx!pxOXw4x2XmGszxqKSXAovQkJx-
JyBTV988c1AXpEDJn9K0UNJwwjhVEcDTvbulL5Dw$ ). A number of
> these look like all the right instructions are coming out, but
> they aren't "in the right order." That is, I see some tests
> expect something along the lines of
>
> %2:_(s16) = G_TRUNC
> %3:_(s10) = G_TRUNC %2:_(s16)
> %4:_(s10) = G_SEXT_INREG %3:_(s10)
> %5:_(s16) = G_SEXT %4:_(s10)
>
> That is, each instruction feeds nicely into the next one.
> But what I'm actually seeing is reordered:
>
> %5:_(s16) = G_SEXT %4:_(s10)
> %2:_(s16) = G_TRUNC
> %4:_(s10) = G_SEXT_INREG %3:_(s10)
> %3:_(s10) = G_TRUNC %2:_(s16)
>
> Now, I can certainly patch up the test to match the order I see,
> but... Is that actually correct? Naively it looks like we have
> uses preceding defs, which just looks weird.

That’s definitely broken!

>
> If the order I'm seeing is wrong (and it's just a standard dump
> of a MachineFunction) then can somebody fix it?

Are these the tests you marked as FIXME in the PR you’ve linked?

Thanks, Quentin! Some of them have other more functional differences,
which are probably normal things where I can fix the test to match.
The ones that have just the ordering problem look like this set:

FewerElementsPhi
WidenScalarBuildVector
WidenSEXTINREG
NarrowSEXTINREG

Thanks!
--paulr

Hi Paul,

This is a bug with the test.

The various legalizerhelper methods expect that the MIRBuilder is pointing to the input MI when doing their expansions.
This is set automatically when you use the legalizeInstrStep API, but if you use another method directly, looks like this is the client responsibility to set it.

The patch below fixes your problem (you’ll have to do it for all of them).

It would be nice to have some kind of asserts here.

@Amara what do you think?

diff --git a/llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp b/llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp
index 5d31b6e876b8…dd7927acd6f5 100644
— a/llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp
+++ b/llvm/unittests/CodeGen/GlobalISel/LegalizerHelperTest.cpp
@@ -965,6 +965,9 @@ TEST_F(AArch64GISelMITest, MoreElementsAnd) {
}

TEST_F(AArch64GISelMITest, FewerElementsPhi) {

Hi Quentin,

Thanks for the investigation and sample patch! I will apply that in this week’s 10% time, so I should have a new patch up on Friday.

Thanks,

–paulr