[PATCH] D23207: If-conversion incorrectly calculates liveness of redefined registers

[Matthias Braun via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

LGTM. That was a subtle issue, thanks.

Some nitpicks below:


================
Comment at: lib/CodeGen/IfConversion.cpp:1434-1435
@@ -1433,4 +1433,4 @@
 
-  // Initialize liveins to the first BB. These are potentially redefined by
-  // predicated instructions.
+  // Initialize Redefs to be the live-outs from the split block. These are
+  // potentially redefined by predicated instructions.
   Redefs.init(TRI);
----------------
I don't understand the new comment (the old comment was pretty bad too though).

How about something in the line of:
```
// - BB2 live-in regs need implicit uses before being redefined by BB1 instructions.
// - BB1 live-out regs need implicit uses before being redefined by BB2 instructions;
//    We start with BB1 live-ins so we have the live-out regs after tracking the BB1 instructions.
```

================
Comment at: test/CodeGen/Hexagon/ifcvt-impuse-livein.mir:1
@@ +1,2 @@
+# RUN: llc -march=hexagon -run-pass if-converter %s -o - | FileCheck %s
+
----------------
Nice small and to the point test!

Would be nice if you could add one sentence about what is tested.
I would also recommend a # CHECK-LABEL: name: foo (just in case more tests get added later).

================
Comment at: test/CodeGen/Hexagon/ifcvt-impuse-livein.mir:16
@@ +15,3 @@
+    successors: %bb.1, %bb.2
+    liveins: %r0, %r1, %r2, %p1
+        J2_jumpf %p1, %bb.1, implicit-def %pc
----------------
The %r1 livein looks unnecessary.


Repository:
  rL LLVM

https://reviews.llvm.org/D23207