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
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.
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
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?
>
> 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:
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.