From 2012083871b75ca032fefaec739ce1f280a8f3de Mon Sep 17 00:00:00 2001 From: JF Bastien Date: Fri, 4 Dec 2015 01:18:17 +0000 Subject: [PATCH] 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 --- lib/Target/X86/X86InstrInfo.cpp | 16 +++++++-- test/CodeGen/X86/cmpxchg-clobber-flags.ll | 43 ++++++++++++++++++++--- test/CodeGen/X86/peephole-na-phys-copy-folding.ll | 8 +++-- 3 files changed, 57 insertions(+), 10 deletions(-) diff --git a/lib/Target/X86/X86InstrInfo.cpp b/lib/Target/X86/X86InstrInfo.cpp index 12da3a9319e..e9d36f8ce2f 100644 --- 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 c129128b5fa..791edba89c4 100644 --- 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 438bf8ddf4c..891a925611c 100644 --- 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]]] -- 2.11.0