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

[shchenz]

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.

[shchenz]

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?

[shchenz]

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.

[steven.zhang]

In D71833#1794987, @shchenz wrote:

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]

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.

[steven.zhang]

Transfer to Zheng to continue working on this.

[shchenz]

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

[lkail]

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]

In D71833#1795362, @lkail wrote:

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]

fix result mask MB > ME cases.

[steven.zhang]

LGTM with some minor nit.

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp

924

s/cases/case
s/not/no

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp

for (MachineInstr &MI : MBB) {
         if (FinalMask.isNullValue()) {
           bool Is64Bit = (MI.getOpcode() == PPC::RLWINM8 ||
                           MI.getOpcode() == PPC::RLWINM8o);
 
           LLVM_DEBUG(dbgs() << "Replace Instr: ");
           LLVM_DEBUG(MI.dump());
 
           if (MI.getOpcode() == PPC::RLWINM || MI.getOpcode() == PPC::RLWINM8) {
             // Replace MI with "LI 0"
             MI.RemoveOperand(4);
             MI.RemoveOperand(3);
             MI.RemoveOperand(2);
             MI.getOperand(1).ChangeToImmediate(0);
             MI.setDesc(TII->get(Is64Bit ? PPC::LI8 : PPC::LI));
           } else {
             // Replace MI with "ANDIo reg, 0"
             MI.RemoveOperand(4);
             MI.RemoveOperand(3);
             MI.getOperand(2).setImm(0);
             MI.setDesc(TII->get(Is64Bit ? PPC::ANDI8o : PPC::ANDIo));
           }
           Simplified = true;
           NumRotatesCollapsed++;
 
           LLVM_DEBUG(dbgs() << "With: ");
           LLVM_DEBUG(MI.dump());
-        } else if (isRunOfOnes((unsigned)(FinalMask.getZExtValue()), NewMB,
-                               NewME) || SrcMaskFull) {
+        } else if ((isRunOfOnes((unsigned)(FinalMask.getZExtValue()), NewMB,
+                               NewME) && NewMB <= NewME)|| SrcMaskFull) {
+          // Here we only handle MBMI <= MEMI case, so NewMB must be no bigger
+          // than NewME. Otherwise we get a 64 bit value after folding, but MI
+          // return a 32 bit value.
+
           // If FoldingReg has only one use and it it not RLWINMo and
           // RLWINM8o, safe to delete its def SrcMI. Otherwise keep it.
           if (MRI->hasOneNonDBGUse(FoldingReg) &&
               (SrcMI->getOpcode() == PPC::RLWINM ||
                SrcMI->getOpcode() == PPC::RLWINM8)) {
             ToErase = SrcMI;
             LLVM_DEBUG(dbgs() << "Delete dead instruction: ");
             LLVM_DEBUG(SrcMI->dump());

llvm/test/CodeGen/PowerPC/fold-rlwinm.mir

bb.0.entry:
     %1:gprc = COPY %0.sub_32:g8rc
     %2:gprc = RLWINM %1:gprc, 10, 5, 31
     ; CHECK: %2:gprc = RLWINM %1, 10, 5, 31
     %3:gprc = RLWINM %2:gprc, 10, 30, 5
     ; CHECK: %3:gprc = RLWINM %2, 10, 30, 5
     BLR8 implicit $lr8, implicit $rm
 ...
 ---
+name: testFoldRLWINMResultWrapped
+#CHECK : name : testFoldRLWINMResultWrapped
+tracksRegLiveness: true
+body: |
+  bb.0.entry:
+    liveins: $x3
+    %0:g8rc = COPY $x3
+    %1:gprc = COPY %0.sub_32:g8rc
+    %2:gprc = RLWINM %1:gprc, 10, 20, 10
+    ; CHECK: %2:gprc = RLWINM %1, 10, 20, 10
+    %3:gprc = RLWINM %2:gprc, 10, 0, 31
+    ; CHECK: %3:gprc = RLWINM %2, 10, 0, 31
+    BLR8 implicit $lr8, implicit $rm
+...
+---
 name: testFoldRLWINMMultipleUses
 #CHECK : name : testFoldRLWINMMultipleUses
 tracksRegLiveness: true
 body: |
   bb.0.entry:
     liveins: $x3
     %0:g8rc = COPY $x3
     %1:gprc = COPY %0.sub_32:g8rc