This is an archive of the discontinued LLVM Phabricator instance.

[CGAtomic] Lyft strong requirement for remaining compare_exchange combinations
ClosedPublic

Authored by bruno on Apr 28 2021, 5:09 PM.

Details

Summary

Follow up on 431e3138a and complete the other possible combinations.

Besides enforcing the new behavior it also mitigates several TSAN false positives
when combining orders that used to be stronger and therefore not allowed.

Diff Detail

Event Timeline

bruno created this revision.Apr 28 2021, 5:09 PM
bruno updated this revision to Diff 342561.May 3 2021, 3:10 PM
bruno added a reviewer: rjmccall.

Fix failing tests and clang-format issue.

jfb accepted this revision.May 3 2021, 4:39 PM
This revision is now accepted and ready to land.May 3 2021, 4:39 PM

Your change is correct, but I think there's a deeper flaw in this code.

clang/lib/CodeGen/CGAtomic.cpp
460

Is this logic correct post-C++17? It looks like if you request a relaxed success order and an acquire failure order, this will use a relaxed order for both. That used to be okay because of the no-stronger restriction, but I think it's just invalid now, and all the conditionality needs to be removed.

jfb added inline comments.May 3 2021, 5:46 PM
clang/lib/CodeGen/CGAtomic.cpp
460

Hmm, I kinda feel like the whole switch thing is silly and we might want to just seq_cst any non-constexpr values. Barring that, yeah we should just adopt the C++17 change unconditionally, since the previous behavior was kinda silly.

bruno added inline comments.May 3 2021, 7:08 PM
clang/lib/CodeGen/CGAtomic.cpp
460

@rjmccall You are right about relaxed/acquire, I plan to address that in the future, I noticed this exact problem this morning while trying to fix D99434.

@jfb why is it silly? Isn't it possible to use runtime values that are still legit and would benefit from the switch? Or did you just mean for the specific case in John's comment?

Don't mind JF, he's just forgotten that C++ lacks the ability to turn the typically constant value arguments passed to functions like std::atomic::compare_exchange_strong into actual compile-time constants to the builtin used within those functions, so his advice would mean emitting all atomics in C++ as sequentially consistent.

Bruno, I think it would be straightforward to just fix this now; you just need to make creating AcquireBB and SeqCstBB unconditional. If you think there's a good reason not to do that in one patch, I won't insist, but it feels to me like the right patch.

jfb added a comment.May 4 2021, 2:01 PM

Don't mind JF, he's just forgotten that C++ lacks the ability to turn the typically constant value arguments passed to functions like std::atomic::compare_exchange_strong into actual compile-time constants to the builtin used within those functions, so his advice would mean emitting all atomics in C++ as sequentially consistent.

Ugh right you are ☹️
I have a small soapbox about making this stuff a template parameter but I'll keep it to myself.

bruno added a comment.May 4 2021, 4:23 PM

Don't mind JF, he's just forgotten that C++ lacks the ability to turn the typically constant value arguments passed to functions like std::atomic::compare_exchange_strong into actual compile-time constants to the builtin used within those functions, so his advice would mean emitting all atomics in C++ as sequentially consistent.

C++ often pushing towards the harsh reality!

Bruno, I think it would be straightforward to just fix this now; you just need to make creating AcquireBB and SeqCstBB unconditional. If you think there's a good reason not to do that in one patch, I won't insist, but it feels to me like the right patch.

Fair enough, will do! Thanks for bringing it up.

I have a small soapbox about making this stuff a template parameter but I'll keep it to myself.

Why not a small paper? :D

Don't mind JF, he's just forgotten that C++ lacks the ability to turn the typically constant value arguments passed to functions like std::atomic::compare_exchange_strong into actual compile-time constants to the builtin used within those functions, so his advice would mean emitting all atomics in C++ as sequentially consistent.

Ugh right you are ☹️
I have a small soapbox about making this stuff a template parameter but I'll keep it to myself.

It's almost funny how much IR this generates at -O0 (and a fix here will make it worse, of course). I wish it worked any other way. :)

bruno updated this revision to Diff 343556.May 6 2021, 7:20 PM
bruno retitled this revision from [CGAtomic] Handle memory_order_consume failing order on compare_exchange to [CGAtomic] Lyft strong requirement for remaining compare_exchange combinations.
bruno edited the summary of this revision. (Show Details)

Update after @rjmccall comments!

bruno updated this revision to Diff 343558.May 6 2021, 7:25 PM

Reduce the diff in a test.

rjmccall accepted this revision.May 6 2021, 8:58 PM

LGTM, except that the commit message should say "Lift", not "Lyft". ;)

bruno added a comment.May 6 2021, 9:00 PM

LGTM, except that the commit message should say "Lift", not "Lyft". ;)

LOL, thanks!

bruno closed this revision.May 6 2021, 9:28 PM

Forgot to add the Differential tag, my bad. Closed by https://reviews.llvm.org/rG819e0d105e84