[PHIElimination] Update the regression test for PR16508

[bjope]

Summary

When PR16508 was solved (in rL185363) a regression test was
added as test/CodeGen/PowerPC/2013-07-01-PHIElimBug.ll.
I discovered that the test case no longer reproduced the
scenario from PR16508. This problem could have been amended
by adding an extra RUN line with "-O1" (or possibly "-O0"),
but instead I added a mir-reproducer

test/CodeGen/PowerPC/2013-07-01-PHIElimBug.mir

to get a reproducer that is less sensitive to changes in
earlier passes (including O-level).

While being at it I also corrected a code comment in
PHIElimination::EliminatePHINodes that has been incorrect
since the related bugfix from rL185363.

[MatzeB]

LGTM, thanks! Maybe we can even drop the .ll test now?

[bjope]

In D52553#1248187, @MatzeB wrote:

LGTM, thanks! Maybe we can even drop the .ll test now?

Original plan was to remove the .ll test. Then I read about LiveVariables being deprecated, and since I use livevars it in the new .mir test I though that perhaps that test case will be invalid in the future.

Now when I've got the bigger picture about the history regarding LiveVariable vs LiveIntervals in this pass I have this suggestion:

1. I'll remove the .ll test case in this patch.
2. I'll add comments new .mir test case (above the RUN line), describing that this is a reduced reproducer based on the old .ll test case.

Given the information in the .mirtest (and the commit msg) it should be quite easy to dig up the old .ll test case again, if needed in the future, for example if livevars is removed.

I'll update the patch according to the above.

[bjope]

Removed the .ll test case (and updated comments in the new .mir test) as suggested in the review comments.

[MatzeB]

LGTM

llvm/trunk/lib/CodeGen/PHIElimination.cpp

bool PHIElimination::runOnMachineFunction(MachineFunction &MF) {
   MF.getProperties().set(MachineFunctionProperties::Property::NoPHIs);
 
   return Changed;
 }
 
 /// EliminatePHINodes - Eliminate phi nodes by inserting copy instructions in
 /// predecessor basic blocks.
 bool PHIElimination::EliminatePHINodes(MachineFunction &MF,
-                                             MachineBasicBlock &MBB) {
   if (MBB.empty() || !MBB.front().isPHI())
     return false;   // Quick exit for basic blocks without PHIs.
 
-  // Get an iterator to the first instruction after the last PHI node (this may
-  // also be the end of the basic block).
+  // Get an iterator to the last PHI node.
   MachineBasicBlock::iterator LastPHIIt =
     std::prev(MBB.SkipPHIsAndLabels(MBB.begin()));
 
   while (MBB.front().isPHI())
     LowerPHINode(MBB, LastPHIIt);
 
   return true;
 }

llvm/trunk/test/CodeGen/PowerPC/2013-07-01-PHIElimBug.ll

-; RUN: llc < %s -verify-machineinstrs | FileCheck %s
-
-target datalayout = "E-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-f128:128:128-v128:128:128-n32:64"
-target triple = "powerpc64-unknown-linux-gnu"
-
-@g_51 = external global [8 x i32], align 4
-
-; CHECK: func_7
-
-; Function Attrs: nounwind
-define fastcc void @func_7() #0 {
-entry:
-  %arrayidx638 = getelementptr inbounds [3 x [1 x i32]], [3 x [1 x i32]]* undef, i64 0, i64 1, i64 0
-  br i1 undef, label %for.cond940, label %if.end1018
-
-for.cond940:                                      ; preds = %for.cond940, %if.else876
-  %l_655.1 = phi i32* [ getelementptr inbounds ([8 x i32], [8 x i32]* @g_51, i64 0, i64 6), %entry ], [ %l_654.0, %for.cond940 ]
-  %l_654.0 = phi i32* [ null, %entry ], [ %arrayidx638, %for.cond940 ]
-  %exitcond = icmp eq i32 undef, 20
-  br i1 %exitcond, label %if.end1018, label %for.cond940
-
-if.end1018:                                       ; preds = %for.end957, %for.end834
-  %l_655.3.ph33 = phi i32* [ %l_655.1, %for.cond940 ], [ getelementptr inbounds ([8 x i32], [8 x i32]* @g_51, i64 0, i64 6), %entry ]
-  store i32 0, i32* %l_655.3.ph33, align 4
-  ret void
-}
-
-attributes #0 = { nounwind "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "unsafe-fp-math"="false" "use-soft-float"="false" }

llvm/trunk/test/CodeGen/PowerPC/2013-07-01-PHIElimBug.mir

+# RUN: llc -mtriple powerpc64-unknown-linux-gnu -run-pass livevars -run-pass phi-node-elimination -verify-machineinstrs -o - %s | FileCheck %s
+
+# This test case was originally known as
+#   test/CodeGen/PowerPC/2013-07-01-PHIElimBug.ll
+#
+# It was discovered that the original test case no longer reproduced the bug
+# from PR16508 (solved in rL185363). That could have been resolved by adding
+# -O1 (or possibly -O0) to the run line, but instead the test case was
+# converted into this .mir test case. Having it as a .mir test should make it
+# less sensitive to changes in earlier passes.
+
+--- |
+  target datalayout = "E-m:e-i64:64-n32:64"
+  target triple = "powerpc64-unknown-linux-gnu"
+
+  @g_51 = external global [8 x i32], align 4
+  define void @func_7() {
+    bb.0:
+      ret void
+  }
+...
+---
+name:            func_7
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $x2
+
+    %0:g8rc_and_g8rc_nox0 = ADDIStocHA $x2, @g_51
+    %1:g8rc_and_g8rc_nox0 = LDtocL @g_51, killed %0, implicit $x2 :: (load 8)
+    %2:gprc = LI 0
+    %3:crrc = CMPLWI killed %2, 0
+    BCC 76, killed %3, %bb.2
+
+  bb.1:
+    %4:g8rc = ADDI8 killed %1, 24
+    B %bb.4
+
+  bb.2:
+    %5:g8rc = ADDI8 killed %1, 24
+    %6:g8rc = LI8 0
+    %7:crrc = CMPLWI undef %8:gprc, 20
+
+  bb.3:
+    %9:g8rc = PHI %5, %bb.2, %10, %bb.3
+    %10:g8rc = PHI %6, %bb.2, undef %11:g8rc, %bb.3
+    BCC 68, %7, %bb.3
+    B %bb.4
+
+  bb.4:
+    %12:g8rc_and_g8rc_nox0 = PHI %4, %bb.1, %9, %bb.3
+    %13:g8rc = LI8 0
+    STW8 killed %13, 0, killed %12 :: (store 4)
+    BLR8 implicit $lr8, implicit $rm
+
+...
+
+# Original TR (and 2013-07-01-PHIElimBug.ll) was about verifier errors for bb.3.
+#
+# I got a feeling that we also need to have some checks to see that # the code
+# is correct in some way. Hopefully this test case is stable enough to verify
+# the full MIR like this.
+#
+# CHECK:       bb.0:
+# CHECK:         %0:g8rc_and_g8rc_nox0 = ADDIStocHA $x2, @g_51
+# CHECK-NEXT:    %1:g8rc_and_g8rc_nox0 = LDtocL @g_51, killed %0, implicit killed $x2 :: (load 8)
+# CHECK-NEXT:    %2:gprc = LI 0
+# CHECK-NEXT:    %3:crrc = CMPLWI killed %2, 0
+# CHECK-NEXT:    BCC 76, killed %3, %bb.2
+# CHECK:       bb.1:
+# CHECK:         %4:g8rc = ADDI8 killed %1, 24
+# CHECK-NEXT:    %16:g8rc_and_g8rc_nox0 = COPY killed %4
+# CHECK-NEXT:    B %bb.4
+# CHECK:       bb.2:
+# CHECK:         %5:g8rc = ADDI8 killed %1, 24
+# CHECK-NEXT:    %6:g8rc = LI8 0
+# CHECK-NEXT:    %7:crrc = CMPLWI undef %8:gprc, 20
+# CHECK-NEXT:    %14:g8rc = COPY killed %5
+# CHECK-NEXT:    %15:g8rc = COPY killed %6
+# CHECK:       bb.3:
+# CHECK:         %10:g8rc = COPY killed %15
+# CHECK-NEXT:    %9:g8rc = COPY killed %14
+# CHECK-NEXT:    %14:g8rc = COPY killed %10
+# CHECK-NEXT:    %15:g8rc = IMPLICIT_DEF
+# CHECK-NEXT:    %16:g8rc_and_g8rc_nox0 = COPY killed %9
+# CHECK-NEXT:    BCC 68, %7, %bb.3
+# CHECK-NEXT:    B %bb.4
+# CHECK:       bb.4:
+# CHECK:         %12:g8rc_and_g8rc_nox0 = COPY killed %16
+# CHECK-NEXT:    %13:g8rc = LI8 0
+# CHECK-NEXT:    STW8 killed %13, 0, killed %12 :: (store 4)
+# CHECK-NEXT:    BLR8 implicit $lr8, implicit $rm