<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Oct 27, 2014 at 1:52 PM, Chris Bieneman <span dir="ltr"><<a href="mailto:beanz@apple.com" target="_blank">beanz@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">A few notes.<br>
<br>
According to Chandler's related RFC (<a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-September/077081.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-September/077081.html</a>) we do not have a requirement that all platforms support <atomic>.<br></blockquote><div><br></div><div>I quickly chatted with Chandler and he mentioned that <atomic> is fine so long as we are sticking to std::atomic<integer-type>.</div><div>He wanted to convey that we want to use <atomic> more, not that we can't use it at all today.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
>>! In D5922#11, @majnemer wrote:<br>
> I'm afraid not.  Because your implementation uses operations which are not atomic, it has undefined behavior in the eyes of C++11.<br>
><br>
> C++11 [intro.multithread]p21:<br>
> The execution of a program contains a data race if it contains two conflicting actions in different threads, at least one of which is not atomic, and neither happens before the other. Any such data race results in undefined behavior.<br>
<br>
</span>I think you're misconstruing what this means. Your statement would seem to imply that until C++11 nobody ever managed to write thread safe code-- which is absurd.</blockquote><div><br></div><div>I'm not implying that but relying on undefined behavior isn't great either.</div><div><br></div><div>At best, I dislike declaring the variable "volatile".  I would instead prefer casting to volatile and loading, this would make things more explicit.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">There are certainly undefined behaviors in the implementation I've provided, but they are known, and appropriately handled.<br></blockquote><div><br></div><div>I don't think you can "handle undefined behavior" by definition.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Both the MemFences, and the tmp in the else branch of the original patch can be safely removed, and the code can be re-written into a while loop for performance. An example implementation would be:<br>
<br>
```<br>
<span class="">enum InitStatus {<br>
  Done = -1,<br>
  Uninitialized = 0,<br>
  Wait = 1<br>
};<br>
<br>
</span>void llvm::call_once(once_flag& flag, void (*UserFn)(void)) {<br>
  while (flag != Done) {<br>
    if (flag == Wait) {<br></blockquote><div><br></div><div>I would much rather prefer you do a single load of flag instead.  Also, you don't have a fence on the Wait => Done side; I believe this can lead to unfortunate reorderings.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
      ::Sleep(1);<br>
      continue;<br>
    }<br>
<br>
    sys::cas_flag old_val = sys::CompareAndSwap(&flag, Wait, Uninitialized);<br>
    if (old_val == Uninitialized) {<br>
      UserFn();<br>
      sys::MemoryFence();<br>
      flag = Done;<br>
    }<br>
  }<br>
}<br>
```<br>
<br>
I (and some of my colleagues) believe that this is perfectly safe code, and it eliminates unnecessary MemFences (which you raised concerns about). Do you see a problem with this implementation (other than the fact that it doesn't use std::atomic)?<br>
<br>
I don't have any particular opposition to using std::atomic, however since we don't have an enforced requirement I'm not sure we can assume we have it. Please keep in mind I think we still support deploying back to Windows XP.<br>
<br>
<a href="http://reviews.llvm.org/D5922" target="_blank">http://reviews.llvm.org/D5922</a><br>
<br>
<br>
</blockquote></div><br></div></div>