[cfe-dev] FYI, Intel folks might be looking to add the __iso_volatile_Xxx family for MSVC STL <atomic> soon

Tue Jul 30 16:15:18 PDT 2019

> we can worry about optimization, perhaps using _Atomic, later

We can’t use _Atomic because it would not be ABI compatible with our std::atomic; in particular, because we put the spinlock for non-lock-free atomics inside the atomic, for instance. And because that isn’t a thing for some of our supported frontends.

It is possible that there are different intrinsics we could call for Clang that would make you folks happier, but we don’t know what those are or even if they exist at present.

> I think the only downside to using _ReadWriteBarrier, which compiles to an LLVM IR fence, and volatile memory operations

Also be aware that on Windows there is a lot of code from before the C++ memory model existed, that treats volatile loads as having acquire semantics, hence our `/volatile:ms` switch, which is the default on the platforms that do this for `free` (x86 and amd64).

Billy3

________________________________
From: Reid Kleckner <rnk at google.com>
Sent: Tuesday, July 30, 2019 4:10:28 PM
To: Billy O'Neal (VC LIBS) <bion at microsoft.com>
Cc: Friedman, Eli <efriedma at codeaurora.org>; Grang, Mandeep Singh <mgrang at codeaurora.org>; JF Bastien <jfbastien at apple.com>; cfe-dev <cfe-dev at lists.llvm.org>
Subject: Re: [cfe-dev] FYI, Intel folks might be looking to add the __iso_volatile_Xxx family for MSVC STL <atomic> soon

Apparently I did this in March, and then completely forgot about it: http://reviews.llvm.org/rC357220<https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Freviews.llvm.org%2FrC357220&data=02%7C01%7Cbion%40microsoft.com%7C5340e785a6a44de6422b08d715432513%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637001250443295750&sdata=oqdHoI3TSxD2RAyN42UGY8b601x7M9i1a3U8HpOHw40%3D&reserved=0>

I think I still had concerns about whether implementing <atomic> with _ReadWriteBarrier and plain, non-LLVM-atomic, volatile loads and stores is completely correct. I never figured out how to express those concerns, so I guess I'll just raise it to JF and Eli, since they probably know more about the C++ memory model than I do. Are there any correctness problems with the VC++ library <atomic> implementation strategy?

I think the only downside to using _ReadWriteBarrier, which compiles to an LLVM IR fence, and volatile memory operations, is that they won't optimize as well as LLVM-IR-atomic loads and stores. But as long as it's correct, we can worry about optimization, perhaps using _Atomic, later.

On Thu, Jul 25, 2019 at 12:18 PM Billy O'Neal (VC LIBS) <bion at microsoft.com<mailto:bion at microsoft.com>> wrote:
Update: MSVC++ should gain this ability in 2019 Update 4; so it would be nice to enable these as soon as possible. The user in https://developercommunity.visualstudio.com/content/problem/274938/vs2017-1567158p2-stdatomicload-causes-write-access.html<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdevelopercommunity.visualstudio.com%2Fcontent%2Fproblem%2F274938%2Fvs2017-1567158p2-stdatomicload-causes-write-access.html&data=02%7C01%7Cbion%40microsoft.com%7C5340e785a6a44de6422b08d715432513%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637001250443305744&sdata=NCklr3zO%2BMnFYDlUpS8cZJKwKomznWv9tNOHTb5bibg%3D&reserved=0> has been long suffering 🙂

Billy3
________________________________
From: Billy O'Neal (VC LIBS) <bion at microsoft.com<mailto:bion at microsoft.com>>
Sent: Thursday, March 28, 2019 03:13 PM
To: Reid Kleckner <rnk at google.com<mailto:rnk at google.com>>; Friedman, Eli <efriedma at codeaurora.org<mailto:efriedma at codeaurora.org>>; Grang, Mandeep Singh <mgrang at codeaurora.org<mailto:mgrang at codeaurora.org>>
Cc: JF Bastien <jfbastien at apple.com<mailto:jfbastien at apple.com>>; cfe-dev <cfe-dev at lists.llvm.org<mailto:cfe-dev at lists.llvm.org>>
Subject: Re: [cfe-dev] FYI, Intel folks might be looking to add the __iso_volatile_Xxx family for MSVC STL <atomic> soon

>The optimizers may change the order of volatile operations relative to non-volatile operations. This is not Java’s “volatile” and has no cross-thread synchronization behavior.

Right, but if they want to link with MSVC++ generated code they need to at least respect _ReadWriteBarrier<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fcpp%2Fintrinsics%2Freadwritebarrier%3Fview%3Dvs-2017&data=02%7C01%7Cbion%40microsoft.com%7C5340e785a6a44de6422b08d715432513%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637001250443305744&sdata=q62Fw5gzx0524CAIkVOVk%2FjxaVlzdW%2FivxB2RE9bZ0k%3D&reserved=0>.

I can certainly mess with our <atomic> to play more nicely with you folks if you want us calling different builtins instead. <atomic> is entirely library tech for us.

Billy3
________________________________
From: Reid Kleckner <rnk at google.com<mailto:rnk at google.com>>
Sent: Thursday, March 28, 2019 03:08 PM
To: Billy O'Neal (VC LIBS); Friedman, Eli; Grang, Mandeep Singh
Cc: JF Bastien; cfe-dev
Subject: Re: [cfe-dev] FYI, Intel folks might be looking to add the __iso_volatile_Xxx family for MSVC STL <atomic> soon

On Thu, Mar 28, 2019 at 2:28 PM Billy O'Neal (VC LIBS) <bion at microsoft.com<mailto:bion at microsoft.com>> wrote:
>Are you sure we shouldn't be marking these as atomic instead of volatile? Volatile is usually not suitable for anything except

I am implementing <atomic> 🙂

My concern is that volatile in LLVM has nothing to do with atomicity, and I think what you really want is LLVM atomic loads and stores, with real ordering markers of monotonic, seq_cst, release, acquire, etc. This is all described here:
https://llvm.org/docs/LangRef.html#volatile-memory-accesses<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fllvm.org%2Fdocs%2FLangRef.html%23volatile-memory-accesses&data=02%7C01%7Cbion%40microsoft.com%7C5340e785a6a44de6422b08d715432513%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637001250443315740&sdata=7FGbuLn5GZB%2B65qLlsOA29rYjXlvIENbsk1zzjRYAFo%3D&reserved=0>
https://llvm.org/docs/LangRef.html#memory-model-for-concurrent-operations<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fllvm.org%2Fdocs%2FLangRef.html%23memory-model-for-concurrent-operations&data=02%7C01%7Cbion%40microsoft.com%7C5340e785a6a44de6422b08d715432513%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637001250443315740&sdata=ytOttLd9m3RsN9VMBP04voImbNeFHYcfUGB5EQUjyWw%3D&reserved=0>
In particular: "The optimizers may change the order of volatile operations relative to non-volatile operations. This is not Java’s “volatile” and has no cross-thread synchronization behavior."

So, I'm concerned that there is something subtly incorrect about using __iso_volatile_* to implement atomics.

After looking at the xatomic.h source, I think I have a better understanding of what is going on. I added some folks who probably have a better understanding of LLVM IR atomics, and maybe they can help guide the discussion to a simpler implementation of Visual C++ <atomic> that plays well with LLVM. We had a similar discussion about over- or under-emitting dmb fences in this code review: https://reviews.llvm.org/D52807<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD52807&data=02%7C01%7Cbion%40microsoft.com%7C5340e785a6a44de6422b08d715432513%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637001250443325736&sdata=B%2BR32BFNQ57PKyFAKly9624V5Jr02mgIxnvfrMyW%2F2s%3D&reserved=0>.

The Visual C++ headers currently add fences for ARM themselves. The code looks like this: https://reviews.llvm.org/P8137<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FP8137&data=02%7C01%7Cbion%40microsoft.com%7C5340e785a6a44de6422b08d715432513%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637001250443325736&sdata=HipedgnoAk4vkWevTvUdjtfvPyjMuoOnWXtSNK909pE%3D&reserved=0> In small test cases, this generates the appropriate code, a single load followed by a fence.

-----

Completely aside from improving the implementation of <atomic> with clang, I will go ahead and make those __iso_volatile_* intrinsics available everywhere.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20190730/00854fc1/attachment.html>