Skip to content

S
swiftshader
Commit 0e432ac4 by Jim Stichnoth
Options

Subzero: Fix a lowering bug involving xchg and xadd instructions.

The x86-32 xchg and xadd instructions are modeled using two source operands, one of which is a memory operand and the other ultimately a physical register. These instructions have a side effect of modifying both operands. During lowering, we need to specially express that the instruction modifies the Variable operand (since it doesn't appear as the instruction's Dest variable). This makes the register allocator aware of the Variable being multi-def, and prevents it from sharing a register with an overlapping live range. This was being partially expressed by adding a FakeDef instruction. However, FakeDef instructions are still allowed to be dead-code eliminated, and if this happens, the Variable may appear to be single-def, triggering the unsafe register sharing. The solution is to prevent the FakeDef instruction from being eliminated, via a FakeUse instruction. It turns out that the current register allocator isn't aggressive enough to manifest the bug with cmpxchg instructions, but the fix and tests are there just in case. BUG= none R=jvoung@chromium.org Review URL: https://codereview.chromium.org/1020853011
parent 3e5009f6
Showing with 115 additions and 2 deletions
LOWERING.rst
...@@ -218,3 +218,22 @@ instruction. Using pseudocode:: ...@@ -218,3 +218,22 @@ instruction. Using pseudocode::
If ``v_result_high`` is live but ``v_result_low`` is dead, adding ``t1`` as an If ``v_result_high`` is live but ``v_result_low`` is dead, adding ``t1`` as an
argument to ``InstFakeDef`` suffices to keep the ``call`` instruction live. argument to ``InstFakeDef`` suffices to keep the ``call`` instruction live.
Instructions modifying source operands
--------------------------------------
Some native instructions may modify one or more source operands. For example,
the x86 ``xadd`` and ``xchg`` instructions modify both source operands. Some
analysis needs to identify every place a ``Variable`` is modified, and it uses
the presence of a ``Dest`` variable for this analysis. Since ICE instructions
have at most one ``Dest``, the ``xadd`` and ``xchg`` instructions need special
treatment.
A ``Variable`` that is not the ``Dest`` can be marked as modified by adding an
``InstFakeDef``. However, this is not sufficient, as the ``Variable`` may have
no more live uses, which could result in the ``InstFakeDef`` being dead-code
eliminated. The solution is to add an ``InstFakeUse`` as well.
To summarize, for every source ``Variable`` that is not equal to the
instruction's ``Dest``, append an ``InstFakeDef`` and ``InstFakeUse``
instruction to provide the necessary analysis information.
src/IceTargetLoweringX8632.h
...@@ -260,6 +260,7 @@ protected: ...@@ -260,6 +260,7 @@ protected:
Context.insert( Context.insert(
InstFakeDef::create(Func, Eax, llvm::dyn_cast<Variable>(DestOrAddr))); InstFakeDef::create(Func, Eax, llvm::dyn_cast<Variable>(DestOrAddr)));
_set_dest_nonkillable(); _set_dest_nonkillable();
Context.insert(InstFakeUse::create(Func, Eax));
} }
void _cmpxchg8b(OperandX8632Mem *Addr, Variable *Edx, Variable *Eax, void _cmpxchg8b(OperandX8632Mem *Addr, Variable *Edx, Variable *Eax,
Variable *Ecx, Variable *Ebx, bool Locked) { Variable *Ecx, Variable *Ebx, bool Locked) {
...@@ -268,8 +269,10 @@ protected: ...@@ -268,8 +269,10 @@ protected:
// Mark edx, and eax as possibly modified by cmpxchg8b. // Mark edx, and eax as possibly modified by cmpxchg8b.
Context.insert(InstFakeDef::create(Func, Edx)); Context.insert(InstFakeDef::create(Func, Edx));
_set_dest_nonkillable(); _set_dest_nonkillable();
Context.insert(InstFakeUse::create(Func, Edx));
Context.insert(InstFakeDef::create(Func, Eax)); Context.insert(InstFakeDef::create(Func, Eax));
_set_dest_nonkillable(); _set_dest_nonkillable();
Context.insert(InstFakeUse::create(Func, Eax));
} }
void _cvt(Variable *Dest, Operand *Src0, InstX8632Cvt::CvtVariant Variant) { void _cvt(Variable *Dest, Operand *Src0, InstX8632Cvt::CvtVariant Variant) {
Context.insert(InstX8632Cvt::create(Func, Dest, Src0, Variant)); Context.insert(InstX8632Cvt::create(Func, Dest, Src0, Variant));
...@@ -468,17 +471,20 @@ protected: ...@@ -468,17 +471,20 @@ protected:
void _xadd(Operand *Dest, Variable *Src, bool Locked) { void _xadd(Operand *Dest, Variable *Src, bool Locked) {
Context.insert(InstX8632Xadd::create(Func, Dest, Src, Locked)); Context.insert(InstX8632Xadd::create(Func, Dest, Src, Locked));
// The xadd exchanges Dest and Src (modifying Src). // The xadd exchanges Dest and Src (modifying Src).
// Model that update with a FakeDef. // Model that update with a FakeDef followed by a FakeUse.
Context.insert( Context.insert(
InstFakeDef::create(Func, Src, llvm::dyn_cast<Variable>(Dest))); InstFakeDef::create(Func, Src, llvm::dyn_cast<Variable>(Dest)));
_set_dest_nonkillable(); _set_dest_nonkillable();
Context.insert(InstFakeUse::create(Func, Src));
} }
void _xchg(Operand *Dest, Variable *Src) { void _xchg(Operand *Dest, Variable *Src) {
Context.insert(InstX8632Xchg::create(Func, Dest, Src)); Context.insert(InstX8632Xchg::create(Func, Dest, Src));
// The xchg modifies Dest and Src -- model that update with a FakeDef. // The xchg modifies Dest and Src -- model that update with a
// FakeDef/FakeUse.
Context.insert( Context.insert(
InstFakeDef::create(Func, Src, llvm::dyn_cast<Variable>(Dest))); InstFakeDef::create(Func, Src, llvm::dyn_cast<Variable>(Dest)));
_set_dest_nonkillable(); _set_dest_nonkillable();
Context.insert(InstFakeUse::create(Func, Src));
} }
void _xor(Variable *Dest, Operand *Src0) { void _xor(Variable *Dest, Operand *Src0) {
Context.insert(InstX8632Xor::create(Func, Dest, Src0)); Context.insert(InstX8632Xor::create(Func, Dest, Src0));
......
tests_lit/llvm2ice_tests/nacl-atomic-intrinsics.ll
...@@ -919,3 +919,91 @@ not_lock_free: ...@@ -919,3 +919,91 @@ not_lock_free:
; CHECK: ret ; CHECK: ret
; CHECK: add ; CHECK: add
; CHECK: ret ; CHECK: ret
; Test the liveness / register allocation properties of the xadd instruction.
; Make sure we model that the Src register is modified and therefore it can't
; share a register with an overlapping live range, even if the result of the
; xadd instruction is unused.
define void @test_xadd_regalloc() {
entry:
br label %body
body:
%i = phi i32 [ 1, %entry ], [ %i_plus_1, %body ]
%g = bitcast [4 x i8]* @Global32 to i32*
%unused = call i32 @llvm.nacl.atomic.rmw.i32(i32 1, i32* %g, i32 %i, i32 6)
%i_plus_1 = add i32 %i, 1
%cmp = icmp eq i32 %i_plus_1, 1001
br i1 %cmp, label %done, label %body
done:
ret void
}
; O2-LABEL: test_xadd_regalloc
;;; Some register will be used in the xadd instruction.
; O2: lock xadd DWORD PTR {{.*}},[[REG:e..]]
;;; Make sure that register isn't used again, e.g. as the induction variable.
; O2-NOT: [[REG]]
; O2: ret
; Do the same test for the xchg instruction instead of xadd.
define void @test_xchg_regalloc() {
entry:
br label %body
body:
%i = phi i32 [ 1, %entry ], [ %i_plus_1, %body ]
%g = bitcast [4 x i8]* @Global32 to i32*
%unused = call i32 @llvm.nacl.atomic.rmw.i32(i32 6, i32* %g, i32 %i, i32 6)
%i_plus_1 = add i32 %i, 1
%cmp = icmp eq i32 %i_plus_1, 1001
br i1 %cmp, label %done, label %body
done:
ret void
}
; O2-LABEL: test_xchg_regalloc
;;; Some register will be used in the xchg instruction.
; O2: xchg DWORD PTR {{.*}},[[REG:e..]]
;;; Make sure that register isn't used again, e.g. as the induction variable.
; O2-NOT: [[REG]]
; O2: ret
; Same test for cmpxchg.
define void @test_cmpxchg_regalloc() {
entry:
br label %body
body:
%i = phi i32 [ 1, %entry ], [ %i_plus_1, %body ]
%g = bitcast [4 x i8]* @Global32 to i32*
%unused = call i32 @llvm.nacl.atomic.cmpxchg.i32(i32* %g, i32 %i, i32 %i, i32 6, i32 6)
%i_plus_1 = add i32 %i, 1
%cmp = icmp eq i32 %i_plus_1, 1001
br i1 %cmp, label %done, label %body
done:
ret void
}
; O2-LABEL: test_cmpxchg_regalloc
;;; eax and some other register will be used in the cmpxchg instruction.
; O2: lock cmpxchg DWORD PTR {{.*}},[[REG:e..]]
;;; Make sure eax isn't used again, e.g. as the induction variable.
; O2-NOT: eax
; O2: ret
; Same test for cmpxchg8b.
define void @test_cmpxchg8b_regalloc() {
entry:
br label %body
body:
%i = phi i32 [ 1, %entry ], [ %i_plus_1, %body ]
%g = bitcast [8 x i8]* @Global64 to i64*
%i_64 = zext i32 %i to i64
%unused = call i64 @llvm.nacl.atomic.cmpxchg.i64(i64* %g, i64 %i_64, i64 %i_64, i32 6, i32 6)
%i_plus_1 = add i32 %i, 1
%cmp = icmp eq i32 %i_plus_1, 1001
br i1 %cmp, label %done, label %body
done:
ret void
}
; O2-LABEL: test_cmpxchg8b_regalloc
;;; eax and some other register will be used in the cmpxchg instruction.
; O2: lock cmpxchg8b QWORD PTR
;;; Make sure eax/ecx/edx/ebx aren't used again, e.g. as the induction variable.
; O2-NOT: {{eax|ecx|edx|ebx}}
; O2: pop ebx
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment