X86InstrInfo::copyPhysReg: workaround reg liveness

Summary:
computeRegisterLiveness and analyzePhysReg are currently getting
confused about liveness in some cases, breaking copyPhysReg's
calculation of whether AX is dead in some cases. Work around this issue
temporarily by assuming that AX is always live.

See detail in: https://llvm.org/bugs/show_bug.cgi?id=25033#c7
And associated bugs PR24535 PR25033 PR24991 PR24992 PR25201.

This workaround makes the code correct but slightly inefficient, but it
seems to confuse the machine instr verifier which now things EAX was
undefined in some cases where it's being conservatively saved /
restored.

Reviewers: majnemer, sanjoy
Subscribers: llvm-commits
Differential Revision: http://reviews.llvm.org/D15198

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@254680 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/Target/X86/X86InstrInfo.cpp b/lib/Target/X86/X86InstrInfo.cpp
index 12da3a9319e63ea9515477b2e9de4568e40ddfa9..e9d36f8ce2f11a12ef2664cbe580eeedda4a8cdc 100644 (file)
--- a/lib/Target/X86/X86InstrInfo.cpp
+++ b/lib/Target/X86/X86InstrInfo.cpp
@@ -4412,9 +4412,19 @@ void X86InstrInfo::copyPhysReg(MachineBasicBlock &MBB,
     int Pop = is64 ? X86::POP64r : X86::POP32r;
     int AX = is64 ? X86::RAX : X86::EAX;
 
-    bool AXDead = (Reg == AX) ||
-                  (MachineBasicBlock::LQR_Dead ==
-                   MBB.computeRegisterLiveness(&getRegisterInfo(), AX, MI));
+    bool AXDead = (Reg == AX);
+    // FIXME: The above could figure out that AX is dead in more cases with:
+    //          || (MachineBasicBlock::LQR_Dead ==
+    //            MBB.computeRegisterLiveness(&getRegisterInfo(), AX, MI));
+    //
+    //        Unfortunately this is slightly broken, see PR24535 and the likely
+    //        related PR25033 PR24991 PR24992 PR25201. These issues seem to
+    //        showcase sub-register / super-register confusion: a previous kill
+    //        of AH but no kill of AL leads computeRegisterLiveness to
+    //        erroneously conclude that AX is dead.
+    //
+    //        Once fixed, also update cmpxchg-clobber-flags.ll and
+    //        peephole-na-phys-copy-folding.ll.
 
     if (!AXDead)
       BuildMI(MBB, MI, DL, get(Push)).addReg(AX, getKillRegState(true));

diff --git a/test/CodeGen/X86/cmpxchg-clobber-flags.ll b/test/CodeGen/X86/cmpxchg-clobber-flags.ll
index c129128b5fa78996c295c2c913fe0e29cd54d67d..791edba89c4459f5746b4d686579cf1dae2eb0cc 100644 (file)
--- a/test/CodeGen/X86/cmpxchg-clobber-flags.ll
+++ b/test/CodeGen/X86/cmpxchg-clobber-flags.ll
@@ -1,7 +1,14 @@
-; RUN: llc -verify-machineinstrs -mtriple=i386-linux-gnu %s -o - | FileCheck %s -check-prefix=i386
-; RUN: llc -verify-machineinstrs -mtriple=i386-linux-gnu -pre-RA-sched=fast %s -o - | FileCheck %s -check-prefix=i386f
-; RUN: llc -verify-machineinstrs -mtriple=x86_64-linux-gnu %s -o - | FileCheck %s -check-prefix=x8664
-; RUN: llc -verify-machineinstrs -mtriple=x86_64-linux-gnu -pre-RA-sched=fast %s -o - | FileCheck %s -check-prefix=x8664
+; RUN: llc -mtriple=i386-linux-gnu %s -o - | FileCheck %s -check-prefix=i386
+; RUN: llc -mtriple=i386-linux-gnu -pre-RA-sched=fast %s -o - | FileCheck %s -check-prefix=i386f
+; RUN: llc -mtriple=x86_64-linux-gnu %s -o - | FileCheck %s -check-prefix=x8664
+; RUN: llc -mtriple=x86_64-linux-gnu -pre-RA-sched=fast %s -o - | FileCheck %s -check-prefix=x8664
+
+; FIXME: X86InstrInfo::copyPhysReg had code which figured out whether AX was
+;        live or not to avoid save / restore when it's not needed. See FIXME in
+;        that function for more details on which the code is currently
+;        disabled. The extra push/pop are marked below and can be removed once
+;        the issue is fixed.
+;        -verify-machineinstrs should also be added back in the RUN lines above.
 
 declare i32 @foo()
 declare i32 @bar(i64)
@@ -17,22 +24,34 @@ define i64 @test_intervening_call(i64* %foo, i64 %bar, i64 %baz) {
 ; i386-NEXT: movl %edx, 4(%esp)
 ; i386-NEXT: movl %eax, (%esp)
 ; i386-NEXT: calll bar
+; ** FIXME Next line isn't actually necessary. **
+; i386-NEXT: pushl %eax
 ; i386-NEXT: movl [[FLAGS]], %eax
 ; i386-NEXT: addb $127, %al
 ; i386-NEXT: sahf
+; ** FIXME Next line isn't actually necessary. **
+; i386-NEXT: popl %eax
 ; i386-NEXT: jne
 
 ; i386f-LABEL: test_intervening_call:
 ; i386f: cmpxchg8b
 ; i386f-NEXT: movl %eax, (%esp)
 ; i386f-NEXT: movl %edx, 4(%esp)
+; ** FIXME Next line isn't actually necessary. **
+; i386f-NEXT: pushl %eax
 ; i386f-NEXT: seto %al
 ; i386f-NEXT: lahf
 ; i386f-NEXT: movl %eax, [[FLAGS:%.*]]
+; ** FIXME Next line isn't actually necessary. **
+; i386f-NEXT: popl %eax
 ; i386f-NEXT: calll bar
+; ** FIXME Next line isn't actually necessary. **
+; i386f-NEXT: pushl %eax
 ; i386f-NEXT: movl [[FLAGS]], %eax
 ; i386f-NEXT: addb $127, %al
 ; i386f-NEXT: sahf
+; ** FIXME Next line isn't actually necessary. **
+; i386f-NEXT: popl %eax
 ; i386f-NEXT: jne
 
 ; x8664-LABEL: test_intervening_call:
@@ -44,9 +63,13 @@ define i64 @test_intervening_call(i64* %foo, i64 %bar, i64 %baz) {
 ; x8664-NEXT: popq %rax
 ; x8664-NEXT: movq %rax, %rdi
 ; x8664-NEXT: callq bar
+; ** FIXME Next line isn't actually necessary. **
+; x8664-NEXT: pushq %rax
 ; x8664-NEXT: movq [[FLAGS]], %rax
 ; x8664-NEXT: addb $127, %al
 ; x8664-NEXT: sahf
+; ** FIXME Next line isn't actually necessary. **
+; x8664-NEXT: popq %rax
 ; x8664-NEXT: jne
 
   %cx = cmpxchg i64* %foo, i64 %bar, i64 %baz seq_cst seq_cst
@@ -111,9 +134,13 @@ cond.end:
 define i32 @test_feed_cmov(i32* %addr, i32 %desired, i32 %new) {
 ; i386-LABEL: test_feed_cmov:
 ; i386: cmpxchgl
+; ** FIXME Next line isn't actually necessary. **
+; i386-NEXT: pushl %eax
 ; i386-NEXT: seto %al
 ; i386-NEXT: lahf
 ; i386-NEXT: movl %eax, [[FLAGS:%.*]]
+; ** FIXME Next line isn't actually necessary. **
+; i386-NEXT: popl %eax
 ; i386-NEXT: calll foo
 ; i386-NEXT: pushl %eax
 ; i386-NEXT: movl [[FLAGS]], %eax
@@ -123,9 +150,13 @@ define i32 @test_feed_cmov(i32* %addr, i32 %desired, i32 %new) {
 
 ; i386f-LABEL: test_feed_cmov:
 ; i386f: cmpxchgl
+; ** FIXME Next line isn't actually necessary. **
+; i386f-NEXT: pushl %eax
 ; i386f-NEXT: seto %al
 ; i386f-NEXT: lahf
 ; i386f-NEXT: movl %eax, [[FLAGS:%.*]]
+; ** FIXME Next line isn't actually necessary. **
+; i386f-NEXT: popl %eax
 ; i386f-NEXT: calll foo
 ; i386f-NEXT: pushl %eax
 ; i386f-NEXT: movl [[FLAGS]], %eax
@@ -135,9 +166,13 @@ define i32 @test_feed_cmov(i32* %addr, i32 %desired, i32 %new) {
 
 ; x8664-LABEL: test_feed_cmov:
 ; x8664: cmpxchgl
+; ** FIXME Next line isn't actually necessary. **
+; x8664: pushq %rax
 ; x8664: seto %al
 ; x8664-NEXT: lahf
 ; x8664-NEXT: movq %rax, [[FLAGS:%.*]]
+; ** FIXME Next line isn't actually necessary. **
+; x8664-NEXT: popq %rax
 ; x8664-NEXT: callq foo
 ; x8664-NEXT: pushq %rax
 ; x8664-NEXT: movq [[FLAGS]], %rax

diff --git a/test/CodeGen/X86/peephole-na-phys-copy-folding.ll b/test/CodeGen/X86/peephole-na-phys-copy-folding.ll
index 438bf8ddf4c707a50cd94e7cec2a4b3d4df00183..891a925611cfce6b76f0a98c06e8bebb3e657eb5 100644 (file)
--- a/test/CodeGen/X86/peephole-na-phys-copy-folding.ll
+++ b/test/CodeGen/X86/peephole-na-phys-copy-folding.ll
@@ -1,5 +1,7 @@
-; RUN: llc -verify-machineinstrs -mtriple=i386-linux-gnu %s -o - | FileCheck %s
-; RUN: llc -verify-machineinstrs -mtriple=x86_64-linux-gnu %s -o - | FileCheck %s
+; RUN: llc -mtriple=i386-linux-gnu %s -o - | FileCheck %s
+; RUN: llc -mtriple=x86_64-linux-gnu %s -o - | FileCheck %s
+
+; FIXME Add -verify-machineinstrs back when PR24535 is fixed.
 
 ; The peephole optimizer can elide some physical register copies such as
 ; EFLAGS. Make sure the flags are used directly, instead of needlessly using
@@ -137,7 +139,7 @@ f:
 
 ; CHECK-LABEL: test_two_live_flags:
 ; CHECK:       cmpxchg
-; CHECK-NEXT:  seto %al
+; CHECK:       seto %al
 ; CHECK-NEXT:  lahf
 ; Save result of the first cmpxchg into D.
 ; CHECK-NEXT:  mov{{[lq]}} %[[AX:[er]ax]], %[[D:[re]d[xi]]]