Page MenuHomePhabricator

[ARM] Fix for 64-bit CAS expansion on ARM32 with -O0
ClosedPublic

Authored by iid_iunknown on Dec 1 2016, 11:56 AM.

Details

Summary

This patch fixes comparison of 64-bit atomic with its expected value in CMP_SWAP_64 expansion.

Currently, the low words are compared with CMP, while the high words are compared with SBC. SBC expects the carry flag to be set if CMP detects a difference. CMP might leave the carry unset for unequal arguments though if the first one is >= than the second. This might cause the comparison logic to detect false equality.

Example of the broken C++ code:

std::atomic<long long> at(2);

long long ll = 1;
std::atomic_compare_exchange_strong(&at, &ll, 3);

Even though the atomic at and the expected value ll are not equal and atomic_compare_exchange_strong returns false, at is changed to 3.

The patch replaces SBC with CMPEQ.

Diff Detail

Repository
rL LLVM

Event Timeline

iid_iunknown retitled this revision from to [ARM] Fix for 64-bit CAS expansion on ARM32 with -O0.
iid_iunknown updated this object.
iid_iunknown added a reviewer: t.p.northover.
iid_iunknown set the repository for this revision to rL LLVM.
iid_iunknown added subscribers: asl, llvm-commits.
t.p.northover accepted this revision.Dec 1 2016, 1:00 PM
t.p.northover edited edge metadata.
t.p.northover added inline comments.
lib/Target/ARM/ARMExpandPseudoInsts.cpp
937–938

You can probably drop the "MIB =" here. Otherwise everything looks fine though. Thanks for fixing this!

This revision is now accepted and ready to land.Dec 1 2016, 1:00 PM
iid_iunknown edited edge metadata.

Addressing the review remarks.

iid_iunknown marked an inline comment as done.Dec 1 2016, 3:05 PM
iid_iunknown added inline comments.
lib/Target/ARM/ARMExpandPseudoInsts.cpp
937–938

Yes, you are right. I've updated the patch.

Thanks for your review, Tim!

iid_iunknown closed this revision.Dec 1 2016, 3:08 PM
iid_iunknown marked an inline comment as done.