<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">LGTM.</div><div class="gmail_quote"><br></div><div class="gmail_quote">On Tue, Jul 1, 2014 at 2:37 AM, Tim Northover <span dir="ltr"><<a href="mailto:t.p.northover@gmail.com" target="_blank">t.p.northover@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div id=":47k" class="a3s" style="overflow:hidden">Hi Chandler,<br>
<br>
Thanks for looking at the patch!<br>
<div class=""><br>
> This has the potential to be an ABI-breaking change. Does your new pass fall<br>
> back to libcalls in exactly the same places that the old code did? Otherwise,<br>
> depending on how the libctalls are implemented, this could cause a break with<br>
> code compiled previously.<br>
<br>
</div>That's a difficult one. It *does* change when we fall back, but<br>
because of how it happens I think it's probably OK for a variety of<br>
reasons:<br>
<br>
1. The helpers aren't actually implemented on either Linux or OS X.<br>
GCC already inlines __int128 atomic ops so it's arguably *fixing* an<br>
ABI bug.<br></div></blockquote><div><br></div><div>I think this is actually the critical thing that makes this a strict improvement.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div id=":47k" class="a3s" style="overflow:hidden">
<br>
2. We already inline atomic_compare_exchange (on 128-bit types) to<br>
cmpxchg16b when available; this expands the inlining to other<br>
atomicrmw operations. Since these can be mixed, I think any ABI<br>
implementation we're compatible with already has to implement these<br>
calls in terms of cmpxchg16b rather than any global lock. So it<br>
probably doesn't break currently unbroken ABIs.<br>
<br>
3. The change is in the backend, and Clang has its own<br>
MaxAtomicInlineWidth override. It seems a reasonable position that if<br>
you're using LLVM's atomic instructions, you're requesting inlining if<br>
possible.<br></div></blockquote><div><br></div><div>Clang and C++'s ABI concerns are totally separate, I agree. I'm not worried on that front.</div></div><br></div></div>