[PowerPC] replace rlwinm operand 1 with src rlwinm operand 1

[shchenz]

Summary

This is split from https://reviews.llvm.org/D71833.

After folding rlwinm, forget to replace result rlwinm operand 1 with src rlwinm operand 1.

By doing this, we can eliminate more src rlwinm.

[steven.zhang]

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp

894

it is not
s/and/or
it is safe

896

You don't need to check it here. Please move to line 964 and remove it if its use is 0.

[MaskRay]

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp

958

The coding standard uses pre-increment operators.

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

136

This -NOT pattern is too specific. Do you intend to make it general so that it can catch more problems if the exact instruction changes a bit in the future?

[shchenz]

address comments

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

136

Good Idea!

[steven.zhang]

The RLWINMo should also be removed,

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp

962

You don't need to check the opcode any more and it will fix the missing case of

RLWINM
RLWINMo -->

ANDIo

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

136

No. This is NOT right. What you are checking is the removal of RLWINM, instead of %2:gprc. You can do it as:

; CHECK-NOT: RLWINM {regex here}, 27

136–137

Why the RLWINMo is still here ?

[steven.zhang]

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

136–137

Ah, sorry. I didn't notice that, you are replacing it with ANDIo. So, it is fine. But I still don't agree with checking the symbol register number which is vulnerable.

[shchenz]

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

136–137

Here what we check is that original input for MI (%2) is not needed? I am glad if you can point out why Check-not %2 is not good?

[shchenz]

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

136–137

If we do something wrongly in future to change 2% to the result of other opcode, we should still can eliminate it. This can only caught by current CHECK-NOT, but can not checked by CHECK-NOT %2 RLWINM? @steven.zhang?

[steven.zhang]

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

136–137

Your test point should align with your implementation. If it is change to other opcode in the future, it is hard to say if it is right or wrong as it is not your test point. And besides, the symbolic register is subject to change.

[shchenz]

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

136–137

Yes, the test point here is we should eliminate instruction %2:gprc = RLWINM %1:gprc, 27, 5, 10, so IMO it is ok to check if %2:gprc is stilled defined/used. And %2:gprc is got from test case input, so I guess if any change wants to change the input symbolic register, it must be aware that the input symbolic register is used in checking lines.

[shchenz]

update testing point

[steven.zhang]

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp

962

Can we use the check of SrcMI->hasImplicitDef() instead of enumerating the opcode to be more flexible ?

[shchenz]

address @steven.zhang comments

[steven.zhang]

LGTM. But please hold on some days in case someone else has other comments.

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp

for (MachineInstr &MI : MBB) {
         // equal to highBit.
         // If MBSrc - MESrc == 1, we expect a full set mask instead of Null.
         if (SrcMaskFull && (MBSrc - MESrc == 1))
           MaskSrc.setAllBits();
 
         APInt RotatedSrcMask = MaskSrc.rotl(SHMI);
         APInt FinalMask = RotatedSrcMask & MaskMI;
         uint32_t NewMB, NewME;
 
         // If final mask is 0, MI result should be 0 too.
         if (FinalMask.isNullValue()) {
           bool Is64Bit = (MI.getOpcode() == PPC::RLWINM8 ||
                           MI.getOpcode() == PPC::RLWINM8_rec);
 
+          Simplified = true;
+
           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 "ANDI_rec reg, 0"
             MI.RemoveOperand(4);
             MI.RemoveOperand(3);
             MI.getOperand(2).setImm(0);
             MI.setDesc(TII->get(Is64Bit ? PPC::ANDI8_rec : PPC::ANDI_rec));
+            MI.getOperand(1).setReg(SrcMI->getOperand(1).getReg());
+            if (SrcMI->getOperand(1).isKill()) {
+              MI.getOperand(1).setIsKill(true);
+              SrcMI->getOperand(1).setIsKill(false);
+            } else
+              // About to replace MI.getOperand(1), clear its kill flag.
+              MI.getOperand(1).setIsKill(false);
           }
-          Simplified = true;
-          NumRotatesCollapsed++;
 
           LLVM_DEBUG(dbgs() << "With: ");
           LLVM_DEBUG(MI.dump());
         } 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 RLWINM_rec and
-          // RLWINM8_rec, 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());
-          }
-
+          Simplified = true;
           LLVM_DEBUG(dbgs() << "Converting Instr: ");
           LLVM_DEBUG(MI.dump());
 
           uint16_t NewSH = (SHSrc + SHMI) % 32;
           MI.getOperand(2).setImm(NewSH);
           // If SrcMI mask is full, no need to update MBMI and MEMI.
           if (!SrcMaskFull) {
             MI.getOperand(3).setImm(NewMB);
             MI.getOperand(4).setImm(NewME);
           }
           MI.getOperand(1).setReg(SrcMI->getOperand(1).getReg());
           if (SrcMI->getOperand(1).isKill()) {
             MI.getOperand(1).setIsKill(true);
             SrcMI->getOperand(1).setIsKill(false);
           } else
             // About to replace MI.getOperand(1), clear its kill flag.
             MI.getOperand(1).setIsKill(false);
 
-          Simplified = true;
-          NumRotatesCollapsed++;
-
           LLVM_DEBUG(dbgs() << "To: ");
           LLVM_DEBUG(MI.dump());
         }
+        if (Simplified) {
+          // If FoldingReg has no non-debug use and it has no implicit def (it
+          // is not RLWINMO or RLWINM8o), it's safe to delete its def SrcMI.
+          // Otherwise keep it.
+          ++NumRotatesCollapsed;
+          if (MRI->use_nodbg_empty(FoldingReg) && !SrcMI->hasImplicitDef()) {
+            ToErase = SrcMI;
+            LLVM_DEBUG(dbgs() << "Delete dead instruction: ");
+            LLVM_DEBUG(SrcMI->dump());
+          }
+        }
         break;
       }
       }
     }
 
     // If the last instruction was marked for elimination,
     // remove it now.
     if (ToErase) {

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

 #CHECK : name : testFoldRLWINMToZero
 tracksRegLiveness: true
 body: |
   bb.0.entry:
     liveins: $x3
     %0:g8rc = COPY $x3
     %1:gprc = COPY %0.sub_32:g8rc
     %2:gprc = RLWINM %1:gprc, 27, 5, 10
-    ; CHECK: %2:gprc = RLWINM %1, 27, 5, 10
+    ; CHECK-NOT: RLWINM %1,
     %3:gprc = RLWINM %2:gprc, 8, 5, 10
     ; CHECK: %3:gprc = LI 0
     BLR8 implicit $lr8, implicit $rm
 ...
 ---
 name: testFoldRLWINM_recToZero
 #CHECK : name : testFoldRLWINM_recToZero
 tracksRegLiveness: true
 body: |
   bb.0.entry:
     liveins: $x3
     %0:g8rc = COPY $x3
     %1:gprc = COPY %0.sub_32:g8rc
     %2:gprc = RLWINM %1:gprc, 27, 5, 10
-    ; CHECK: %2:gprc = RLWINM %1, 27, 5, 10
+    ; CHECK-NOT: RLWINM %1,
     %3:gprc = RLWINM_rec %2:gprc, 8, 5, 10, implicit-def $cr0
-    ; CHECK: %3:gprc = ANDI_rec %2, 0, implicit-def $cr0
+    ; CHECK: %3:gprc = ANDI_rec %1, 0, implicit-def $cr0
+    BLR8 implicit $lr8, implicit $rm
+...
+---
+name: testFoldRLWINMoToZeroSrcCanNotBeDeleted
+#CHECK : name : testFoldRLWINMoToZeroSrcCanNotBeDeleted
+tracksRegLiveness: true
+body: |
+  bb.0.entry:
+    liveins: $x3
+    %0:g8rc = COPY $x3
+    %1:gprc = COPY %0.sub_32:g8rc
+    %2:gprc = RLWINM_rec %1:gprc, 27, 5, 10, implicit-def $cr0
+    ; CHECK: %2:gprc = RLWINM_rec %1, 27, 5, 10, implicit-def $cr0
+    %3:gprc = RLWINM_rec %2:gprc, 8, 5, 10, implicit-def $cr0
+    ; CHECK: %3:gprc = ANDI_rec %1, 0, implicit-def $cr0
     BLR8 implicit $lr8, implicit $rm
 ...
 ---
 name: testFoldRLWINMInvalidMask
 #CHECK : name : testFoldRLWINMInvalidMask
 tracksRegLiveness: true
 body: |
   bb.0.entry: