[PATCH] D85044: Add __atomic_is_lock_free to compiler-rt

[Louis Dionne via Phabricator via llvm-commits]

ldionne added a subscriber: arichardson.
ldionne added a comment.

In D85044#2432266 <https://reviews.llvm.org/D85044#2432266>, @oontvoo wrote:

> In D85044#2431334 <https://reviews.llvm.org/D85044#2431334>, @ldionne wrote:
>
>> Doesn't D92302 <https://reviews.llvm.org/D92302> also check for the alignment of the pointer? My understanding is that with D92302 <https://reviews.llvm.org/D92302>, we're returning `true` from `atomic_is_lock_free` whenever the implementation in compiler-rt, as of D92302 <https://reviews.llvm.org/D92302>, would be lockfree. Whether that's the very best we can do on the hardware is a different story -- but D92302 <https://reviews.llvm.org/D92302>'s `atomic_is_lockfree` will be consistent with the actual implementation. Am i mistaken?
>
> Yes,  D92302 <https://reviews.llvm.org/D92302> provides an implementation of is_lock_free that is  consistent with the **current** _atomic_load_* implementations.
> (it's a no change in behaviour)
>
>> So, basically, this patch does the above, but also allows using a lockfree implementation in more cases, is that it?
>
> Yes.
>
>> I'm trying to figure out whether it makes sense to land D92302 <https://reviews.llvm.org/D92302>, and then this patch, since this one seems a lot more involved (and still has TODOs). Please let me know if I'm not getting this right, I'm a bit out of my depth trying to read this patch TBH.
>
> The two TODOs in this patch was the attempt to simply this so that we have the basic framework in place before filling in more meaty details (ARMs and odd sizes support) . I suspect we'd do those in subsequent patches.

Got it, thanks a lot for the clarifications. Unless we're confident that we can move forward with this patch in the near future, I would start by doing D92302 <https://reviews.llvm.org/D92302> since it solves real problems for libc++ (and Apple platforms to the extent that we're currently shipping a broken compiler-rt). @arichardson There are some tests in this patch, perhaps you could grab them? @oontvoo would you be OK with us trying to land D92302 <https://reviews.llvm.org/D92302> and then rebasing your patch on top?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85044/new/

https://reviews.llvm.org/D85044