On Thu, Mar 29, 2018 at 4:10 PM, Nicolas Boichat <drink...@chromium.org> wrote:
> On Fri, Mar 30, 2018 at 2:26 AM, Matt Turner <matts...@gmail.com> wrote:
>> On Thu, Mar 29, 2018 at 1:31 AM, Nicolas Boichat <drink...@chromium.org>
>> wrote:
>>> From: Nicolas Boichat <drink...@chromium.org>
>>>
>>> When compiling with LLVM 6.0, the test fails to detect that
>>> -latomic is actually required, as the atomic call is inlined.
>>>
>>> In the code itself (src/util/disk_cache.c), we see this pattern:
>>> p_atomic_add(cache->size, - (uint64_t)size);
>>> where cache->size is an uint64_t *, and results in the following
>>> link time error without -latomic:
>>> src/util/disk_cache.c:628: error: undefined reference to
>>> '__atomic_fetch_add_8'
>>>
>>> Fix the configure/meson test to replicate this pattern, which then
>>> correctly realizes the need for -latomic.
>>>
>>> Signed-off-by: Nicolas Boichat <drink...@chromium.org>
>>> ---
>>>
>>> Changes since v1:
>>> - Updated meson.build as well (untested)
>>>
>>> configure.ac | 6 ++++--
>>> meson.build | 6 ++++--
>>> 2 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/configure.ac b/configure.ac
>>> index e874f8ebfb2..eff9a0ef88f 100644
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -445,9 +445,11 @@ if test "x$GCC_ATOMIC_BUILTINS_SUPPORTED" = x1; then
>>> AC_MSG_CHECKING(whether -latomic is needed)
>>> AC_LINK_IFELSE([AC_LANG_SOURCE([[
>>> #include <stdint.h>
>>> - uint64_t v;
>>> + struct {
>>> + uint64_t* v;
>>
>> I wouldn't care expect that you put the * with the v in the Meson case. :)
>
> Argh ,-( I'll send a v3, let's see if anyone has further comments, first.
>
>> Also, on what platform does this occur?
>
> This is ARC++ (Android 32-bit x86) with clang version:
> Android (4639204 based on r316199) clang version 6.0.1
> (https://android.googlesource.com/toolchain/clang
> 279c0d3a962121a6d1d535e7b0b5d9d36d3c829d)
> (https://android.googlesource.com/toolchain/llvm
> aadd87ffb6a2eafcb577913073d46b20195a9cdc) (based on LLVM 6.0.1svn)
>
>> Looking at this code, I would expect it to behave the same as before.
>> Do you have an idea why this fixes it, or why the original code didn't
>> work? I'm guess it's about the compiler's ability to recognize that it
>> knows the location of the variable.
>
> With the original code, objdump looks like this:
>
> 08048400 <main>:
> 8048400: 53 push %ebx
> 8048401: 56 push %esi
> 8048402: e8 00 00 00 00 call 8048407 <main+0x7>
> 8048407: 5e pop %esi
> 8048408: 81 c6 ed 1b 00 00 add $0x1bed,%esi
> 804840e: 31 c0 xor %eax,%eax
> 8048410: 31 d2 xor %edx,%edx
> 8048412: 31 c9 xor %ecx,%ecx
> 8048414: 31 db xor %ebx,%ebx
> 8048416: f0 0f c7 8e 24 00 00 lock cmpxchg8b 0x24(%esi)
> 804841d: 00
> 804841e: 5e pop %esi
> 804841f: 5b pop %ebx
> 8048420: c3 ret
>
> Looks like LLVM figures out that &v is constant, and uses some 64-bit
> atomic swap operations on it directly.
>
> With the updated code (building with -latomic, it fails otherwise)
> 08048480 <main>:
> 8048480: 53 push %ebx
> 8048481: 83 ec 08 sub $0x8,%esp
> 8048484: e8 00 00 00 00 call 8048489 <main+0x9>
> 8048489: 5b pop %ebx
> 804848a: 81 c3 6b 1b 00 00 add $0x1b6b,%ebx
> 8048490: 83 ec 08 sub $0x8,%esp
> 8048493: 6a 02 push $0x2
> 8048495: ff b3 8c 10 00 00 pushl 0x108c(%ebx)
> 804849b: e8 05 00 00 00 call 80484a5 <__atomic_load_8>
> 80484a0: 83 c4 18 add $0x18,%esp
> 80484a3: 5b pop %ebx
> 80484a4: c3 ret
>
> I think the the code is trying to protect both x.v (address) _and_ its
> value *x.v? Or maybe LLVM does not see the pattern... (I don't see why
> cmpxchg8b wouldn't work here too, otherwise...)
>
> Actually, the test can be made simpler, by just using:
> uint64_t *v;
> ...
> __atomic_load_n(v, ...
>
> But then it does not match the usage pattern in the code, so I feel a
> little bit more confident that the current test will actually capture
> when -latomic is needed.
Yeah. That makes sense, and seems like a good idea to me.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
