subject:"\[Mesa\-dev\] \[PATCH, v2\] CHROMIUM\: configure.ac\/meson.build\: Fix \-latomic test"

Re: [Mesa-dev] [PATCH, v2] CHROMIUM: configure.ac/meson.build: Fix -latomic test

2018-04-04 Thread Matt Turner

On Thu, Mar 29, 2018 at 4:10 PM, Nicolas Boichat  wrote:
> On Fri, Mar 30, 2018 at 2:26 AM, Matt Turner  wrote:
>> On Thu, Mar 29, 2018 at 1:31 AM, Nicolas Boichat  
>> wrote:
>>> From: Nicolas Boichat 
>>>
>>> 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 
>>> ---
>>>
>>> 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 
>>> -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 :
>  8048400:   53  push   %ebx
>  8048401:   56  push   %esi
>  8048402:   e8 00 00 00 00  call   8048407 
>  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 00lock cmpxchg8b 0x24(%esi)
>  804841d:   00
>  804841e:   5e  pop%esi
>  804841f:   5b  pop%ebx
>  8048420:   c3  ret
>
> Looks like LLVM figures out that  is constant, and uses some 64-bit
> atomic swap operations on it directly.
>
> With the updated code (building with -latomic, it fails otherwise)
> 08048480 :
>  8048480:   53  push   %ebx
>  8048481:   83 ec 08sub$0x8,%esp
>  8048484:   e8 00 00 00 00  call   8048489 
>  8048489:   5b  pop%ebx
>  804848a:   81 c3 6b 1b 00 00   add$0x1b6b,%ebx
>  8048490:   83 ec 08sub$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 18add$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

Re: [Mesa-dev] [PATCH, v2] CHROMIUM: configure.ac/meson.build: Fix -latomic test

2018-03-29 Thread Nicolas Boichat

On Fri, Mar 30, 2018 at 2:26 AM, Matt Turner  wrote:
> On Thu, Mar 29, 2018 at 1:31 AM, Nicolas Boichat  
> wrote:
>> From: Nicolas Boichat 
>>
>> 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 
>> ---
>>
>> 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 
>> -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 :
 8048400:   53  push   %ebx
 8048401:   56  push   %esi
 8048402:   e8 00 00 00 00  call   8048407 
 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 00lock cmpxchg8b 0x24(%esi)
 804841d:   00
 804841e:   5e  pop%esi
 804841f:   5b  pop%ebx
 8048420:   c3  ret

Looks like LLVM figures out that  is constant, and uses some 64-bit
atomic swap operations on it directly.

With the updated code (building with -latomic, it fails otherwise)
08048480 :
 8048480:   53  push   %ebx
 8048481:   83 ec 08sub$0x8,%esp
 8048484:   e8 00 00 00 00  call   8048489 
 8048489:   5b  pop%ebx
 804848a:   81 c3 6b 1b 00 00   add$0x1b6b,%ebx
 8048490:   83 ec 08sub$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 18add$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.

Thanks,
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH, v2] CHROMIUM: configure.ac/meson.build: Fix -latomic test

2018-03-29 Thread Matt Turner

On Thu, Mar 29, 2018 at 1:31 AM, Nicolas Boichat  wrote:
> From: Nicolas Boichat 
>
> 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 
> ---
>
> 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 
> -uint64_t v;
> +struct {
> +uint64_t* v;

I wouldn't care expect that you put the * with the v in the Meson case. :)

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.

Also, on what platform does this occur?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH, v2] CHROMIUM: configure.ac/meson.build: Fix -latomic test

2018-03-29 Thread Nicolas Boichat

And now I left the CHROMIUM tag, sorry ,-(

On Thu, Mar 29, 2018 at 4:31 PM, Nicolas Boichat  wrote:
> From: Nicolas Boichat 
>
> 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 
> ---
>
> 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 
> -uint64_t v;
> +struct {
> +uint64_t* v;
> +} x;
>  int main() {
> -return (int)__atomic_load_n(, __ATOMIC_ACQUIRE);
> +return (int)__atomic_load_n(x.v, __ATOMIC_ACQUIRE);
>  }]])], GCC_ATOMIC_BUILTINS_NEED_LIBATOMIC=no, 
> GCC_ATOMIC_BUILTINS_NEED_LIBATOMIC=yes)
>  AC_MSG_RESULT($GCC_ATOMIC_BUILTINS_NEED_LIBATOMIC)
>  if test "x$GCC_ATOMIC_BUILTINS_NEED_LIBATOMIC" = xyes; then
> diff --git a/meson.build b/meson.build
> index f210eeb2530..cd567dc000e 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -850,8 +850,10 @@ if cc.compiles('int main() { int n; return 
> __atomic_load_n(, __ATOMIC_ACQUIRE)
># as ARM.
>if not cc.links('''#include 
>   int main() {
> -   uint64_t n;
> -   return (int)__atomic_load_n(, __ATOMIC_ACQUIRE);
> +   struct {
> + uint64_t *v;
> +   } x;
> +   return (int)__atomic_load_n(x.v, __ATOMIC_ACQUIRE);
>   }''',
>name : 'GCC atomic builtins required -latomic')
>  dep_atomic = cc.find_library('atomic')
> --
> 2.17.0.rc1.321.gba9d0f2565-goog
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH, v2] CHROMIUM: configure.ac/meson.build: Fix -latomic test

2018-03-29 Thread Nicolas Boichat

From: Nicolas Boichat 

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 
---

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 
-uint64_t v;
+struct {
+uint64_t* v;
+} x;
 int main() {
-return (int)__atomic_load_n(, __ATOMIC_ACQUIRE);
+return (int)__atomic_load_n(x.v, __ATOMIC_ACQUIRE);
 }]])], GCC_ATOMIC_BUILTINS_NEED_LIBATOMIC=no, 
GCC_ATOMIC_BUILTINS_NEED_LIBATOMIC=yes)
 AC_MSG_RESULT($GCC_ATOMIC_BUILTINS_NEED_LIBATOMIC)
 if test "x$GCC_ATOMIC_BUILTINS_NEED_LIBATOMIC" = xyes; then
diff --git a/meson.build b/meson.build
index f210eeb2530..cd567dc000e 100644
--- a/meson.build
+++ b/meson.build
@@ -850,8 +850,10 @@ if cc.compiles('int main() { int n; return 
__atomic_load_n(, __ATOMIC_ACQUIRE)
   # as ARM.
   if not cc.links('''#include 
  int main() {
-   uint64_t n;
-   return (int)__atomic_load_n(, __ATOMIC_ACQUIRE);
+   struct {
+ uint64_t *v;
+   } x;
+   return (int)__atomic_load_n(x.v, __ATOMIC_ACQUIRE);
  }''',
   name : 'GCC atomic builtins required -latomic')
 dep_atomic = cc.find_library('atomic')
-- 
2.17.0.rc1.321.gba9d0f2565-goog

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH, v2] CHROMIUM: configure.ac/meson.build: Fix -latomic test

Re: [Mesa-dev] [PATCH, v2] CHROMIUM: configure.ac/meson.build: Fix -latomic test

Re: [Mesa-dev] [PATCH, v2] CHROMIUM: configure.ac/meson.build: Fix -latomic test

Re: [Mesa-dev] [PATCH, v2] CHROMIUM: configure.ac/meson.build: Fix -latomic test

[Mesa-dev] [PATCH, v2] CHROMIUM: configure.ac/meson.build: Fix -latomic test

5 matches

Site Navigation

Mail list logo

Footer information