[llvm-dev] Q. about GlobalISel and order of instructions

[Quentin Colombet via llvm-dev]

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) {
+
+  setUp();
+
   if (!TM)
     return;
 
@@ -1027,6 +1030,7 @@ TEST_F(AArch64GISelMITest, FewerElementsPhi) {
   // Add some use instruction after the phis.
   B.buildAnd(PhiTy, Phi.getReg(0), Phi.getReg(0));
 
+  B.setInstr(*Phi);
   EXPECT_EQ(LegalizerHelper::LegalizeResult::Legalized,
             Helper.fewerElementsVector(*Phi, 0, v2s32));


Cheers,
-Quentin
> On Jan 29, 2021, at 12:00 PM, paul.robinson at sony.com wrote:
> 
> 
> 
>> -----Original Message-----
>> From: Quentin Colombet <qcolombet at apple.com>
>> Sent: Friday, January 29, 2021 1:32 PM
>> To: Robinson, Paul <paul.robinson at sony.com>
>> Cc: llvm-dev at lists.llvm.org
>> Subject: Re: [llvm-dev] Q. about GlobalISel and order of instructions
>> 
>> Hi Paul,
>> 
>> Thanks for the heads-up!
>> 
>>> On Jan 28, 2021, at 8:12 PM, via llvm-dev <llvm-dev at lists.llvm.org>
>> wrote:
>>> 
>>> 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
> 
>> 
>> Cheers,
>> -Quentin
>> 
>>> It will sure
>>> make a lot less churn in the unittest.
>>> 
>>> Thanks,
>>> --paulr
>>> 
>>> 
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org
>>> https://urldefense.com/v3/__https://lists.llvm.org/cgi-
>> bin/mailman/listinfo/llvm-
>> dev__;!!JmoZiZGBv3RvKRSx!pxOXw4x2XmGszxqKSXAovQkJx-
>> JyBTV988c1AXpEDJn9K0UNJwwjhVEcDTsB1HBaAQ$
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210129/b55eb1f0/attachment.html>