Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(45)

Issue 1020853011: Subzero: Fix a lowering bug involving xchg and xadd instructions. (Closed)

Created:
5 years, 9 months ago by Jim Stichnoth
Modified:
5 years, 9 months ago
CC:
native-client-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

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 Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=0e432ac4c72b8226c5cf12636e5ff277136dcb74

Patch Set 1 #

Patch Set 2 : Similar for cmpxchg and cmpxchg8b. Add documentation. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+115 lines, -2 lines) Patch
M LOWERING.rst View 1 1 chunk +19 lines, -0 lines 0 comments Download
M src/IceTargetLoweringX8632.h View 1 3 chunks +8 lines, -2 lines 0 comments Download
M tests_lit/llvm2ice_tests/nacl-atomic-intrinsics.ll View 1 1 chunk +88 lines, -0 lines 0 comments Download

Messages

Total messages: 5 (1 generated)
Jim Stichnoth
5 years, 9 months ago (2015-03-24 18:21:25 UTC) #2
Jim Stichnoth
Added similar fixes for the cmpxchg instructions, and updated docs.
5 years, 9 months ago (2015-03-24 22:20:42 UTC) #3
jvoung (off chromium)
lgtm
5 years, 9 months ago (2015-03-24 22:37:13 UTC) #4
Jim Stichnoth
5 years, 9 months ago (2015-03-24 22:39:19 UTC) #5
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as
0e432ac4c72b8226c5cf12636e5ff277136dcb74 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698