Page MenuHomePhabricator

[PowerPC] if value type is changed after folding rlwinm, stop folding
ClosedPublic

Authored by shchenz on Dec 23 2019, 3:45 AM.

Details

Summary

%1:g8rc = RLWINM8 %0:g8rc, 0, 16, 9
%2:g8rc = RLWINM8 killed %1:g8rc, 0, 0, 31

->

%2:g8rc = RLWINM8 %0:g8rc, 0, 16, 9

The above folding is wrong. Before transformation, %2:g8rc is 32 bit value. After transformation, %2:g8rc becomes a 64 bit value.

Diff Detail

Event Timeline

steven.zhang created this revision.Dec 23 2019, 3:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2019, 3:45 AM
steven.zhang edited the summary of this revision. (Show Details)Dec 23 2019, 3:46 AM
steven.zhang edited the summary of this revision. (Show Details)Dec 23 2019, 3:54 AM

Thanks for finding the bug in the testcases. I have modified them in NFC patch https://reviews.llvm.org/rG79b3325be0b016fdc1a2c55bce65ec9f1e5f4eb6

Could you please help to be more specific about the invalid case in the description?

Below c code gives a valid result:

#include <stdio.h>

#define rlwinm( output, input, sh, mb, me ) \
   __asm__( "rlwinm %0, %1, %2, %3, %4" \
          : "=r"(output) \
          : "r"(input), "i"(sh), "i"(mb), "i"(me) \
          : )

int main()
{
   unsigned long y,z,t ;

   int i = 0;

   for (unsigned x = 0; x < 4294967295; x++)  {
     rlwinm( y, x, 27, 30, 10) ;
     rlwinm( z, y, 19, 0, 12);
     rlwinm(t, x, 14, 11, 12);
     if (z != t) {
       printf("0x%016lX -> 0x%016lX, 0x%016lX\n", x, y, z);
       printf("0x%016lX -> 0x%016lX\n", x, t);
       i = 100;
       break;
    }
   }
   if (i == 0)
     printf("valid\n");
   else
     printf("invalid\n");
   return 0;
}

So I guess

%2:gprc = RLWINM %1:gprc, 27, 30, 10
%3:gprc = RLWINM %2:gprc, 19, 0, 12

should always be equal to

%3:gprc = RLWINM %1, 14, 11, 12

for type i32 and i64 also?

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp
904

We can delete the SrcMI only if it is not a record form?

Seems there is indeed some bug for current folding. If the wrap relationship between mb and me for folding result rlwinm is different with he wrap relationship in MI.

rlwinm1 reg1, sh1, mb1, me1 -> rlwinm2 reg2, sh2, mb2, me2

If (mb1 > me1 && mb2 < me2), then rlwinm1 get 64 bit value, rlwinm2 get 32 bit value;
if (mb1 < me1 && mb2 > me2), then rlwinm1 get 32 bit value, rlwinm2 get 64 bit value.

The above two cases should be wrong. But I think the case in the description should be a valid transform.

Thanks for finding the bug in the testcases. I have modified them in NFC patch https://reviews.llvm.org/rG79b3325be0b016fdc1a2c55bce65ec9f1e5f4eb6

Could you please help to be more specific about the invalid case in the description?

Below c code gives a valid result:

#include <stdio.h>

#define rlwinm( output, input, sh, mb, me ) \
   __asm__( "rlwinm %0, %1, %2, %3, %4" \
          : "=r"(output) \
          : "r"(input), "i"(sh), "i"(mb), "i"(me) \
          : )

int main()
{
   unsigned long y,z,t ;

   int i = 0;

   for (unsigned x = 0; x < 4294967295; x++)  {
     rlwinm( y, x, 27, 30, 10) ;
     rlwinm( z, y, 19, 0, 12);
     rlwinm(t, x, 14, 11, 12);
     if (z != t) {
       printf("0x%016lX -> 0x%016lX, 0x%016lX\n", x, y, z);
       printf("0x%016lX -> 0x%016lX\n", x, t);
       i = 100;
       break;
    }
   }
   if (i == 0)
     printf("valid\n");
   else
     printf("invalid\n");
   return 0;
}

So I guess

%2:gprc = RLWINM %1:gprc, 27, 30, 10
%3:gprc = RLWINM %2:gprc, 19, 0, 12

should always be equal to

%3:gprc = RLWINM %1, 14, 11, 12

for type i32 and i64 also?

This is the bad transformation:

%1:g8rc = RLWINM8 %0:g8rc, 0, 16, 9
%2:g8rc = RLWINM8 killed %1:g8rc, 0, 0, 31

->

%2:g8rc = RLWINM8 %0:g8rc, 0, 16, 9
steven.zhang marked an inline comment as done.Dec 23 2019, 6:28 PM

We need another patch to fix the dead instruction removal for record form I think.

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp
904

Yes, but it miss to correct the reg of the record form. I suggest another patch to correct it together with the kill flag.

Transfer to Zheng to continue working on this.

shchenz commandeered this revision.Dec 23 2019, 6:30 PM
shchenz edited reviewers, added: steven.zhang; removed: shchenz.

Thanks for finding out this bug. As discussed, I will take this patch.

shchenz edited the summary of this revision. (Show Details)Dec 23 2019, 6:32 PM
lkail added a subscriber: lkail.Dec 23 2019, 6:46 PM

I would suggest to retitle it to be more descriptive about code behavior, like 'Add check' blah blah. Describe what bugs it can fix in summary field.

shchenz retitled this revision from [PowerPC] Fix some bugs of the rlwinm folding to [PowerPC] if value type is changed after folding rlwinm, stop folding.Dec 25 2019, 5:48 PM

I would suggest to retitle it to be more descriptive about code behavior, like 'Add check' blah blah. Describe what bugs it can fix in summary field.

Good idea!

shchenz updated this revision to Diff 235309.Dec 25 2019, 6:06 PM

fix result mask MB > ME cases.

steven.zhang accepted this revision.Dec 25 2019, 6:11 PM

LGTM with some minor nit.

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp
924

s/cases/case
s/not/no

This revision is now accepted and ready to land.Dec 25 2019, 6:11 PM
This revision was automatically updated to reflect the committed changes.