[llvm] r254680 - X86InstrInfo::copyPhysReg: workaround reg liveness
JF Bastien via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 3 17:18:18 PST 2015
Author: jfb
Date: Thu Dec 3 19:18:17 2015
New Revision: 254680
URL: http://llvm.org/viewvc/llvm-project?rev=254680&view=rev
Log:
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
Modified:
llvm/trunk/lib/Target/X86/X86InstrInfo.cpp
llvm/trunk/test/CodeGen/X86/cmpxchg-clobber-flags.ll
llvm/trunk/test/CodeGen/X86/peephole-na-phys-copy-folding.ll
Modified: llvm/trunk/lib/Target/X86/X86InstrInfo.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86InstrInfo.cpp?rev=254680&r1=254679&r2=254680&view=diff
==============================================================================
--- llvm/trunk/lib/Target/X86/X86InstrInfo.cpp (original)
+++ llvm/trunk/lib/Target/X86/X86InstrInfo.cpp Thu Dec 3 19:18:17 2015
@@ -4412,9 +4412,19 @@ void X86InstrInfo::copyPhysReg(MachineBa
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));
Modified: llvm/trunk/test/CodeGen/X86/cmpxchg-clobber-flags.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/cmpxchg-clobber-flags.ll?rev=254680&r1=254679&r2=254680&view=diff
==============================================================================
--- llvm/trunk/test/CodeGen/X86/cmpxchg-clobber-flags.ll (original)
+++ llvm/trunk/test/CodeGen/X86/cmpxchg-clobber-flags.ll Thu Dec 3 19:18:17 2015
@@ -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* %
; 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* %
; 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, i
; 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, i
; 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
Modified: llvm/trunk/test/CodeGen/X86/peephole-na-phys-copy-folding.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/peephole-na-phys-copy-folding.ll?rev=254680&r1=254679&r2=254680&view=diff
==============================================================================
--- llvm/trunk/test/CodeGen/X86/peephole-na-phys-copy-folding.ll (original)
+++ llvm/trunk/test/CodeGen/X86/peephole-na-phys-copy-folding.ll Thu Dec 3 19:18:17 2015
@@ -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]]]
More information about the llvm-commits
mailing list