thr3ads.net - llvm dev - [llvm-dev] Q. about GlobalISel and order of instructions [Feb 2021]

If this information is useful, please help other people find it:
Share via:

via llvm-dev

2021-Jan-29 20:00 UTC

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

> -----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$

Quentin Colombet via llvm-dev

2021-Jan-29 22:23 UTC

head link

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

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>

via llvm-dev

2021-Feb-01 15:13 UTC

head link

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

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

From: Quentin Colombet <qcolombet at apple.com>
Sent: Friday, January 29, 2021 5:24 PM
To: Robinson, Paul <paul.robinson at sony.com>
Cc: llvm-de >> LLVM Developers' List <llvm-dev at
lists.llvm.org>; Amara Emerson <aemerson at apple.com>
Subject: Re: [llvm-dev] Q. about GlobalISel and order of instructions

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<mailto:paul.robinson
at sony.com> wrote:




-----Original Message-----
From: Quentin Colombet <qcolombet at apple.com<mailto:qcolombet at
apple.com>>
Sent: Friday, January 29, 2021 1:32 PM
To: Robinson, Paul <paul.robinson at sony.com<mailto:paul.robinson at
sony.com>>
Cc: llvm-dev at lists.llvm.org<mailto: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<mailto: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<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<mailto: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/20210201/6bd41ebb/attachment-0001.html>

llvm dev - Feb 2021 - Q. about GlobalISel and order of instructions

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

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

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