Page MenuHomePhabricator

[PowerPC] Implement XL compatible behavior of __compare_and_swap
ClosedPublic

Authored by lkail on Mon, Jul 19, 11:02 PM.

Details

Summary

According to https://www.ibm.com/docs/en/xl-c-and-cpp-aix/16.1?topic=functions-compare-swap-compare-swaplp
XL's __compare_and_swap has a weird behavior that

In either case, the contents of the memory location specified by addr are copied into the memory location specified by old_val_addr.

(unlike c11 atomic_compare_exchange specified in http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf)

This patch let clang's implementation follow this behavior.

Diff Detail

Event Timeline

lkail created this revision.Mon, Jul 19, 11:02 PM
lkail requested review of this revision.Mon, Jul 19, 11:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jul 19, 11:02 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
lkail retitled this revision from [PowerPC] Correct behavior of __compare_and_swap to [PowerPC] Implement XL compatible behavior of __compare_and_swap.Mon, Jul 19, 11:02 PM
lkail edited the summary of this revision. (Show Details)
lkail edited the summary of this revision. (Show Details)Mon, Jul 19, 11:05 PM
jsji added a comment.EditedWed, Jul 21, 2:56 PM

Doesn't look good enough to me, the assembly code sequence generated is not clean enough.

lkail added a comment.Wed, Jul 21, 7:26 PM

Doesn't look good enough to me, the assembly code sequence generated is not clean enough.

I'm assuming you mean the second stdcx., that looks like a historical issue which exists for 13yrs. According to https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html and what xlc generates for CAS, it should not exist.

commit 166d6cb1fad159b1aedb3801ecaecb62000979d1
Author: Dale Johannesen <dalej@apple.com>
Date:   Mon Aug 25 18:53:26 2008 +0000

    It's important for the cmp-and-swap to balance
    loads and stores but it's even more important for
    it to store the right value.:(
    
    llvm-svn: 55319
lkail updated this revision to Diff 360699.EditedWed, Jul 21, 9:59 PM

Discussed with @jsji offline about the details of codegen and inspect XL's codegen at different opt level, add an opt test from jinsong to demonstrate the store can be eliminated.

Herald added a project: Restricted Project. · View Herald TranscriptWed, Jul 21, 9:59 PM
lkail updated this revision to Diff 360721.Thu, Jul 22, 12:54 AM
jsji accepted this revision as: jsji.Thu, Jul 22, 7:52 AM

that looks like a historical issue which exists for 13yrs

Hmm... OK.. Let us leave it as it is for now.

Thanks for the new tests.

This revision is now accepted and ready to land.Thu, Jul 22, 7:52 AM
This revision was landed with ongoing or failed builds.Thu, Jul 22, 6:16 PM
This revision was automatically updated to reflect the committed changes.