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

Tue Aug 27 17:39:18 PDT 2019

Hmmm what version of Clang should have this change?

failure: BION-X299 devcrt tests\CPP\Dev11_863628_atomic_compare_exchange /nologo /W4 /w14061 /w14242 /w14582 /w14583 /w14587 /w14588 /w14265 /w14365 /w14749 /w14841 /w14842 /w15038 /sdl /WX /Zc:strictStrings /D_ENABLE_STL_INTERNAL_CHECK /D_ENFORCE_FACET_SPECIALIZATIONS=1 (clang-cl) -fno-ms-compatibility -fno-delayed-template-parsing /EHsc /MD /std:c++14 24
###   Test executable: D:\msvc\src\tools\perl\bin\perl.exe
###    with arguments: -S "D:\msvc\src\qa\vc\libs\devcrt\run.pl"
### Working directory: D:\msvc\src\qa\vc\libs\devcrt\tests\CPP\Dev11_863628_atomic_compare_exchange
### Test exited with code 0x00000003
### Test ran for 0:00:00:02.4496707
### Environment:
set CLANG_TARGET=-m32
set HAS_EDG=yes
set INCLUDE=D:\msvc\binaries\x86ret\inc;D:\msvc\src\ExternalAPIs\Windows\10\sdk\inc;D:\msvc\src\ExternalAPIs\UnifiedCRT\inc;D:\msvc\src\qa\vc\libs\devcrt\include
set LIB=D:\msvc\binaries\x86ret\lib\i386;D:\msvc\src\ExternalAPIs\Windows\10\sdk\lib\x86;D:\msvc\src\ExternalAPIs\UnifiedCRT\lib\i386;D:\msvc\src\ExternalAPIs\NetFX\v4.5\lib\x86;D:\msvc\src\vctools\nonship\cuda\v9.2\lib\x64
set PATH=D:\msvc\binaries\x86ret\bin\i386;D:\msvc\src\qa\vc\shared\testenv\bin;D:\msvc\src\tools\perl\bin;D:\msvc\src\vctools\nonship\clangllvm\x86\bin;D:\msvc\src\vctools\nonship\cuda\v9.2\bin;C:\Program Files\Python37\Scripts\;C:\Program Files\Python37\;C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\Windows\System32\OpenSSH\;C:\Program Files\PuTTY\;C:\Program Files\dotnet\;C:\Program Files\Microsoft SQL Server\130\Tools\Binn\;C:\Program Files\Microsoft SQL Server\Client SDK\ODBC\170\Tools\Binn\;C:\Program Files\CMake\bin;C:\Program Files\LLVM\bin;C:\Program Files (x86)\NVIDIA Corporation\PhysX\Common;C:\Program Files\Git\cmd;C:\Users\bion\AppData\Local\Microsoft\WindowsApps
set PM_CL=/nologo /W4 /w14061 /w14242 /w14582 /w14583 /w14587 /w14588 /w14265 /w14365 /w14749 /w14841 /w14842 /w15038 /sdl /WX /Zc:strictStrings /D_ENABLE_STL_INTERNAL_CHECK /D_ENFORCE_FACET_SPECIALIZATIONS=1 -fno-ms-compatibility -fno-delayed-template-parsing /EHsc /MD /std:c++14
set PM_COMPILER=clang-cl
set TARGET_ARCHITECTURE=x86
### CLI output:
Executing: "D:\msvc\src\qa\vc\shared\testenv\bin\run.pl"
RUN.PL line 41:  _CONFIG_PREFIX  NOT SET
RUN.PL line 61:  Building..
RUN.PL line 90:  Using default build step
RUN.PM line 746:  Executing: "clang-cl -m32  /nologo /W4 /w14061 /w14242 /w14582 /w14583 /w14587 /w14588 /w14265 /w14365 /w14749 /w14841 /w14842 /w15038 /sdl /WX /Zc:strictStrings /D_ENABLE_STL_INTERNAL_CHECK /D_ENFORCE_FACET_SPECIALIZATIONS=1 -fno-ms-compatibility -fno-delayed-template-parsing /EHsc /MD /std:c++14   .\test.cpp   /c".
RUN.PM line 749:  Execution of: "clang-cl -m32  /nologo /W4 /w14061 /w14242 /w14582 /w14583 /w14587 /w14588 /w14265 /w14365 /w14749 /w14841 /w14842 /w15038 /sdl /WX /Zc:strictStrings /D_ENABLE_STL_INTERNAL_CHECK /D_ENFORCE_FACET_SPECIALIZATIONS=1 -fno-ms-compatibility -fno-delayed-template-parsing /EHsc /MD /std:c++14   .\test.cpp   /c" returned "0".
RUN.PM line 746:  Executing: "link  .\test.obj  /out:Dev11_863628_atomic_compare_exchange.exe".
Microsoft (R) Incremental Linker Version 14.24.26504.99
Copyright (C) Microsoft Corporation.  All rights reserved.

test.obj : error LNK2019: unresolved external symbol ___iso_volatile_load64 referenced in function "public: __int64 __thiscall std::_Atomic_storage<__int64,8>::load(void)const " (?load@?$_Atomic_storage at _J$07 at std@@QBE_JXZ)
Dev11_863628_atomic_compare_exchange.exe : fatal error LNK1120: 1 unresolved externals
RUN.PM line 749:  Execution of: "link  .\test.obj  /out:Dev11_863628_atomic_compare_exchange.exe" returned "96".
RUN.PM line 1067 - Error:  link failed.  Cascading.

RUN.PM: Test Cascaded.

Thanks!

Billy O’Neal
Visual C++ Libraries

________________________________
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/20190828/49ea95bd/attachment.html>