* [PATCH] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 @ 2021-02-02 19:12 H.J. Lu 2021-03-01 13:30 ` Carlos O'Donell 0 siblings, 1 reply; 15+ messages in thread From: H.J. Lu @ 2021-02-02 19:12 UTC (permalink / raw) To: libc-alpha Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use "movq reg64, mem64" to store 64-bit constant in TCB. --- sysdeps/x86_64/nptl/tls.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h index 20f0958780..9ec8703e45 100644 --- a/sysdeps/x86_64/nptl/tls.h +++ b/sysdeps/x86_64/nptl/tls.h @@ -269,6 +269,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, asm volatile ("movl %0,%%fs:%P1" : \ : IMM_MODE (value), \ "i" (offsetof (struct pthread, member))); \ + else if (__builtin_constant_p (value) \ + && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value) \ + asm volatile ("movq %0,%%fs:%P1" : \ + : "r" (value), \ + "i" (offsetof (struct pthread, member))); \ else /* 8 */ \ { \ asm volatile ("movq %q0,%%fs:%P1" : \ @@ -294,6 +299,12 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, : IMM_MODE (value), \ "i" (offsetof (struct pthread, member[0])), \ "r" (idx)); \ + else if (__builtin_constant_p (value) \ + && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value) \ + asm volatile ("movq %0,%%fs:%P1(,%q2,8)" : \ + : "r" (value), \ + "i" (offsetof (struct pthread, member[0])), \ + "r" (idx)); \ else /* 8 */ \ { \ asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ -- 2.29.2 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 2021-02-02 19:12 [PATCH] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 H.J. Lu @ 2021-03-01 13:30 ` Carlos O'Donell 2021-03-02 14:21 ` [PATCH v2] " H.J. Lu 0 siblings, 1 reply; 15+ messages in thread From: Carlos O'Donell @ 2021-03-01 13:30 UTC (permalink / raw) To: H.J. Lu, libc-alpha On 2/2/21 2:12 PM, H.J. Lu via Libc-alpha wrote: > Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use > "movq reg64, mem64" to store 64-bit constant in TCB. > --- > sysdeps/x86_64/nptl/tls.h | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h > index 20f0958780..9ec8703e45 100644 > --- a/sysdeps/x86_64/nptl/tls.h > +++ b/sysdeps/x86_64/nptl/tls.h > @@ -269,6 +269,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, > asm volatile ("movl %0,%%fs:%P1" : \ > : IMM_MODE (value), \ > "i" (offsetof (struct pthread, member))); \ > + else if (__builtin_constant_p (value) \ > + && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value) \ > + asm volatile ("movq %0,%%fs:%P1" : \ > + : "r" (value), \ > + "i" (offsetof (struct pthread, member))); \ (1) Move conditional into 'else /* 8 */' section. The blocks of conditionals are predicated on the size of the member we are about to write to. In the block below... > else /* 8 */ \ ... here, we are about to write value into a member that is size 8. Your code changes the logical construction of the code, but in way that makes it more difficult to understand. We previously had: if (sizeof() == 1) else if (sizeof() == 4) else /* Assume 8 */ In your case we must already be in the 'else /* Assume 8 */' because otherwise we've be writing a 64-bit constant into a < 64-bit structure member. I think we should put your code into the else clause. 272 else /* 8 */ \ 273 { \ if (__builtin_constant_p (value) && ([the value is a >32-bit constant]) asm volatile ([use movq reg64, mem64]); else 274 asm volatile ("movq %q0,%%fs:%P1" : \ 275 : IMM_MODE ((uint64_t) cast_to_integer (value)), \ 276 "i" (offsetof (struct pthread, member))); \ 277 }}) (2) What if gcc can't prove it's constant? If the constant is >32-bit, but gcc can't prove it's constant, then don't we try to put a >32-bit constant into a 32-bit immediate? Shouldn't the code be structured the other way around? e.g. else /* 8 */ { if (__builtin_constant_p (value) && ([the value is a <= 32-bit constant]) asm volatile ([use movq imm32, mem64]); else asm volatile ([use movq reg64, mem64]); } This way the code is always correct? > { \ > asm volatile ("movq %q0,%%fs:%P1" : \ > @@ -294,6 +299,12 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, > : IMM_MODE (value), \ > "i" (offsetof (struct pthread, member[0])), \ > "r" (idx)); \ > + else if (__builtin_constant_p (value) \ > + && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value) \ > + asm volatile ("movq %0,%%fs:%P1(,%q2,8)" : \ > + : "r" (value), \ > + "i" (offsetof (struct pthread, member[0])), \ > + "r" (idx)); \ > else /* 8 */ \ > { \ > asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ > -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 2021-03-01 13:30 ` Carlos O'Donell @ 2021-03-02 14:21 ` H.J. Lu 2021-03-08 22:28 ` Carlos O'Donell 0 siblings, 1 reply; 15+ messages in thread From: H.J. Lu @ 2021-03-02 14:21 UTC (permalink / raw) To: Carlos O'Donell; +Cc: GNU C Library [-- Attachment #1: Type: text/plain, Size: 5230 bytes --] On Mon, Mar 1, 2021 at 5:30 AM Carlos O'Donell <carlos@redhat.com> wrote: > > On 2/2/21 2:12 PM, H.J. Lu via Libc-alpha wrote: > > Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use > > "movq reg64, mem64" to store 64-bit constant in TCB. > > --- > > sysdeps/x86_64/nptl/tls.h | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h > > index 20f0958780..9ec8703e45 100644 > > --- a/sysdeps/x86_64/nptl/tls.h > > +++ b/sysdeps/x86_64/nptl/tls.h > > @@ -269,6 +269,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, > > asm volatile ("movl %0,%%fs:%P1" : \ > > : IMM_MODE (value), \ > > "i" (offsetof (struct pthread, member))); \ > > + else if (__builtin_constant_p (value) \ > > + && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value) \ > > + asm volatile ("movq %0,%%fs:%P1" : \ > > + : "r" (value), \ > > + "i" (offsetof (struct pthread, member))); \ > > (1) Move conditional into 'else /* 8 */' section. > > The blocks of conditionals are predicated on the size of the member we are > about to write to. > > In the block below... > > > else /* 8 */ \ > > ... here, we are about to write value into a member that is size 8. > > Your code changes the logical construction of the code, but in way that makes > it more difficult to understand. > > We previously had: > > if (sizeof() == 1) > > else if (sizeof() == 4) > > else /* Assume 8 */ > > In your case we must already be in the 'else /* Assume 8 */' because otherwise > we've be writing a 64-bit constant into a < 64-bit structure member. > > I think we should put your code into the else clause. > > 272 else /* 8 */ \ > 273 { \ > > if (__builtin_constant_p (value) > && ([the value is a >32-bit constant]) > asm volatile ([use movq reg64, mem64]); > else > > 274 asm volatile ("movq %q0,%%fs:%P1" : \ > 275 : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > 276 "i" (offsetof (struct pthread, member))); \ > 277 }}) > > (2) What if gcc can't prove it's constant? > > If the constant is >32-bit, but gcc can't prove it's constant, then don't we > try to put a >32-bit constant into a 32-bit immediate? > > Shouldn't the code be structured the other way around? > > e.g. > > else /* 8 */ > { > if (__builtin_constant_p (value) > && ([the value is a <= 32-bit constant]) > asm volatile ([use movq imm32, mem64]); > else > asm volatile ([use movq reg64, mem64]); > } > > This way the code is always correct? I changed it to if (!__builtin_constant_p (value) \ || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ asm volatile ("movq %q0,%%fs:%P1" : \ : IMM_MODE ((uint64_t) cast_to_integer (value)), \ "i" (offsetof (struct pthread, member))); \ else \ asm volatile ("movq %0,%%fs:%P1" : \ : "r" (value), \ "i" (offsetof (struct pthread, member))); \ > > { \ > > asm volatile ("movq %q0,%%fs:%P1" : \ > > @@ -294,6 +299,12 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, > > : IMM_MODE (value), \ > > "i" (offsetof (struct pthread, member[0])), \ > > "r" (idx)); \ > > + else if (__builtin_constant_p (value) \ > > + && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value) \ > > + asm volatile ("movq %0,%%fs:%P1(,%q2,8)" : \ > > + : "r" (value), \ > > + "i" (offsetof (struct pthread, member[0])), \ > > + "r" (idx)); \ > > else /* 8 */ \ > > { \ > > asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ > > > Here is the v2 patch. OK for master? Thanks. -- H.J. [-- Attachment #2: v2-0001-x86_64-Update-THREAD_SETMEM-THREAD_SETMEM_NC-for-.patch --] [-- Type: text/x-patch, Size: 2417 bytes --] From 4fbc9ab67933d662506a01658d4c81a922e50fdf Mon Sep 17 00:00:00 2001 From: "H.J. Lu" <hjl.tools@gmail.com> Date: Fri, 8 Jan 2021 15:38:14 -0800 Subject: [PATCH v2] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use "movq reg64, mem64" to store 64-bit constant. --- sysdeps/x86_64/nptl/tls.h | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h index 20f0958780..0dbd66209c 100644 --- a/sysdeps/x86_64/nptl/tls.h +++ b/sysdeps/x86_64/nptl/tls.h @@ -271,9 +271,15 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, "i" (offsetof (struct pthread, member))); \ else /* 8 */ \ { \ - asm volatile ("movq %q0,%%fs:%P1" : \ - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ - "i" (offsetof (struct pthread, member))); \ + if (!__builtin_constant_p (value) \ + || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ + asm volatile ("movq %q0,%%fs:%P1" : \ + : IMM_MODE ((uint64_t) cast_to_integer (value)), \ + "i" (offsetof (struct pthread, member))); \ + else \ + asm volatile ("movq %0,%%fs:%P1" : \ + : "r" (value), \ + "i" (offsetof (struct pthread, member))); \ }}) @@ -296,10 +302,17 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, "r" (idx)); \ else /* 8 */ \ { \ - asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ - "i" (offsetof (struct pthread, member[0])), \ - "r" (idx)); \ + if (!__builtin_constant_p (value) \ + || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ + asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ + : IMM_MODE ((uint64_t) cast_to_integer (value)), \ + "i" (offsetof (struct pthread, member[0])), \ + "r" (idx)); \ + else \ + asm volatile ("movq %0,%%fs:%P1(,%q2,8)" : \ + : "r" (value), \ + "i" (offsetof (struct pthread, member[0])), \ + "r" (idx)); \ }}) -- 2.29.2 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 2021-03-02 14:21 ` [PATCH v2] " H.J. Lu @ 2021-03-08 22:28 ` Carlos O'Donell 2021-03-09 0:09 ` [PATCH v3] " H.J. Lu 0 siblings, 1 reply; 15+ messages in thread From: Carlos O'Donell @ 2021-03-08 22:28 UTC (permalink / raw) To: H.J. Lu; +Cc: GNU C Library On 3/2/21 9:21 AM, H.J. Lu wrote: > On Mon, Mar 1, 2021 at 5:30 AM Carlos O'Donell <carlos@redhat.com> wrote: >> >> On 2/2/21 2:12 PM, H.J. Lu via Libc-alpha wrote: >>> Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use >>> "movq reg64, mem64" to store 64-bit constant in TCB. >>> --- >>> sysdeps/x86_64/nptl/tls.h | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h >>> index 20f0958780..9ec8703e45 100644 >>> --- a/sysdeps/x86_64/nptl/tls.h >>> +++ b/sysdeps/x86_64/nptl/tls.h >>> @@ -269,6 +269,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, >>> asm volatile ("movl %0,%%fs:%P1" : \ >>> : IMM_MODE (value), \ >>> "i" (offsetof (struct pthread, member))); \ >>> + else if (__builtin_constant_p (value) \ >>> + && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value) \ >>> + asm volatile ("movq %0,%%fs:%P1" : \ >>> + : "r" (value), \ >>> + "i" (offsetof (struct pthread, member))); \ >> >> (1) Move conditional into 'else /* 8 */' section. >> >> The blocks of conditionals are predicated on the size of the member we are >> about to write to. >> >> In the block below... >> >>> else /* 8 */ \ >> >> ... here, we are about to write value into a member that is size 8. >> >> Your code changes the logical construction of the code, but in way that makes >> it more difficult to understand. >> >> We previously had: >> >> if (sizeof() == 1) >> >> else if (sizeof() == 4) >> >> else /* Assume 8 */ >> >> In your case we must already be in the 'else /* Assume 8 */' because otherwise >> we've be writing a 64-bit constant into a < 64-bit structure member. >> >> I think we should put your code into the else clause. >> >> 272 else /* 8 */ \ >> 273 { \ >> >> if (__builtin_constant_p (value) >> && ([the value is a >32-bit constant]) >> asm volatile ([use movq reg64, mem64]); >> else >> >> 274 asm volatile ("movq %q0,%%fs:%P1" : \ >> 275 : IMM_MODE ((uint64_t) cast_to_integer (value)), \ >> 276 "i" (offsetof (struct pthread, member))); \ >> 277 }}) >> >> (2) What if gcc can't prove it's constant? >> >> If the constant is >32-bit, but gcc can't prove it's constant, then don't we >> try to put a >32-bit constant into a 32-bit immediate? >> >> Shouldn't the code be structured the other way around? >> >> e.g. >> >> else /* 8 */ >> { >> if (__builtin_constant_p (value) >> && ([the value is a <= 32-bit constant]) >> asm volatile ([use movq imm32, mem64]); >> else >> asm volatile ([use movq reg64, mem64]); >> } >> >> This way the code is always correct? > > I changed it to > > if (!__builtin_constant_p (value) \ > || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ > asm volatile ("movq %q0,%%fs:%P1" : \ > : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > "i" (offsetof (struct pthread, member))); \ > else \ > asm volatile ("movq %0,%%fs:%P1" : \ > : "r" (value), \ > "i" (offsetof (struct pthread, member))); \ > >>> { \ >>> asm volatile ("movq %q0,%%fs:%P1" : \ >>> @@ -294,6 +299,12 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, >>> : IMM_MODE (value), \ >>> "i" (offsetof (struct pthread, member[0])), \ >>> "r" (idx)); \ >>> + else if (__builtin_constant_p (value) \ >>> + && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value) \ >>> + asm volatile ("movq %0,%%fs:%P1(,%q2,8)" : \ >>> + : "r" (value), \ >>> + "i" (offsetof (struct pthread, member[0])), \ >>> + "r" (idx)); \ >>> else /* 8 */ \ >>> { \ >>> asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ >>> >> > > Here is the v2 patch. OK for master? > > Thanks. > > From 4fbc9ab67933d662506a01658d4c81a922e50fdf Mon Sep 17 00:00:00 2001 > From: "H.J. Lu" <hjl.tools@gmail.com> > Date: Fri, 8 Jan 2021 15:38:14 -0800 > Subject: [PATCH v2] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 > > Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use > "movq reg64, mem64" to store 64-bit constant. > --- > sysdeps/x86_64/nptl/tls.h | 27 ++++++++++++++++++++------- > 1 file changed, 20 insertions(+), 7 deletions(-) > > diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h > index 20f0958780..0dbd66209c 100644 > --- a/sysdeps/x86_64/nptl/tls.h > +++ b/sysdeps/x86_64/nptl/tls.h > @@ -271,9 +271,15 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, > "i" (offsetof (struct pthread, member))); \ > else /* 8 */ \ > { \ > - asm volatile ("movq %q0,%%fs:%P1" : \ > - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > - "i" (offsetof (struct pthread, member))); \ > + if (!__builtin_constant_p (value) \ Whis is this a "!__builtin_constant_p?" I would have expected: if ([the value is a constant a therefore known quantity] || [it is a 32-bit value]) Perhaps a quick single line comment here explaining the conditionals would help? > + || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ > + asm volatile ("movq %q0,%%fs:%P1" : \ > + : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > + "i" (offsetof (struct pthread, member))); \ > + else \ > + asm volatile ("movq %0,%%fs:%P1" : \ > + : "r" (value), \ > + "i" (offsetof (struct pthread, member))); \ > }}) > > > @@ -296,10 +302,17 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, > "r" (idx)); \ > else /* 8 */ \ > { \ > - asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ > - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > - "i" (offsetof (struct pthread, member[0])), \ > - "r" (idx)); \ > + if (!__builtin_constant_p (value) \ > + || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ > + asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ > + : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > + "i" (offsetof (struct pthread, member[0])), \ > + "r" (idx)); \ > + else \ > + asm volatile ("movq %0,%%fs:%P1(,%q2,8)" : \ > + : "r" (value), \ > + "i" (offsetof (struct pthread, member[0])), \ > + "r" (idx)); \ > }}) > > > -- > 2.29.2 > -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 2021-03-08 22:28 ` Carlos O'Donell @ 2021-03-09 0:09 ` H.J. Lu 2021-03-15 12:49 ` Carlos O'Donell 0 siblings, 1 reply; 15+ messages in thread From: H.J. Lu @ 2021-03-09 0:09 UTC (permalink / raw) To: Carlos O'Donell; +Cc: GNU C Library [-- Attachment #1: Type: text/plain, Size: 7429 bytes --] On Mon, Mar 8, 2021 at 2:28 PM Carlos O'Donell <carlos@redhat.com> wrote: > > On 3/2/21 9:21 AM, H.J. Lu wrote: > > On Mon, Mar 1, 2021 at 5:30 AM Carlos O'Donell <carlos@redhat.com> wrote: > >> > >> On 2/2/21 2:12 PM, H.J. Lu via Libc-alpha wrote: > >>> Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use > >>> "movq reg64, mem64" to store 64-bit constant in TCB. > >>> --- > >>> sysdeps/x86_64/nptl/tls.h | 11 +++++++++++ > >>> 1 file changed, 11 insertions(+) > >>> > >>> diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h > >>> index 20f0958780..9ec8703e45 100644 > >>> --- a/sysdeps/x86_64/nptl/tls.h > >>> +++ b/sysdeps/x86_64/nptl/tls.h > >>> @@ -269,6 +269,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, > >>> asm volatile ("movl %0,%%fs:%P1" : \ > >>> : IMM_MODE (value), \ > >>> "i" (offsetof (struct pthread, member))); \ > >>> + else if (__builtin_constant_p (value) \ > >>> + && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value) \ > >>> + asm volatile ("movq %0,%%fs:%P1" : \ > >>> + : "r" (value), \ > >>> + "i" (offsetof (struct pthread, member))); \ > >> > >> (1) Move conditional into 'else /* 8 */' section. > >> > >> The blocks of conditionals are predicated on the size of the member we are > >> about to write to. > >> > >> In the block below... > >> > >>> else /* 8 */ \ > >> > >> ... here, we are about to write value into a member that is size 8. > >> > >> Your code changes the logical construction of the code, but in way that makes > >> it more difficult to understand. > >> > >> We previously had: > >> > >> if (sizeof() == 1) > >> > >> else if (sizeof() == 4) > >> > >> else /* Assume 8 */ > >> > >> In your case we must already be in the 'else /* Assume 8 */' because otherwise > >> we've be writing a 64-bit constant into a < 64-bit structure member. > >> > >> I think we should put your code into the else clause. > >> > >> 272 else /* 8 */ \ > >> 273 { \ > >> > >> if (__builtin_constant_p (value) > >> && ([the value is a >32-bit constant]) > >> asm volatile ([use movq reg64, mem64]); > >> else > >> > >> 274 asm volatile ("movq %q0,%%fs:%P1" : \ > >> 275 : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > >> 276 "i" (offsetof (struct pthread, member))); \ > >> 277 }}) > >> > >> (2) What if gcc can't prove it's constant? > >> > >> If the constant is >32-bit, but gcc can't prove it's constant, then don't we > >> try to put a >32-bit constant into a 32-bit immediate? > >> > >> Shouldn't the code be structured the other way around? > >> > >> e.g. > >> > >> else /* 8 */ > >> { > >> if (__builtin_constant_p (value) > >> && ([the value is a <= 32-bit constant]) > >> asm volatile ([use movq imm32, mem64]); > >> else > >> asm volatile ([use movq reg64, mem64]); > >> } > >> > >> This way the code is always correct? > > > > I changed it to > > > > if (!__builtin_constant_p (value) \ > > || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ > > asm volatile ("movq %q0,%%fs:%P1" : \ > > : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > > "i" (offsetof (struct pthread, member))); \ > > else \ > > asm volatile ("movq %0,%%fs:%P1" : \ > > : "r" (value), \ > > "i" (offsetof (struct pthread, member))); \ > > > >>> { \ > >>> asm volatile ("movq %q0,%%fs:%P1" : \ > >>> @@ -294,6 +299,12 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, > >>> : IMM_MODE (value), \ > >>> "i" (offsetof (struct pthread, member[0])), \ > >>> "r" (idx)); \ > >>> + else if (__builtin_constant_p (value) \ > >>> + && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value) \ > >>> + asm volatile ("movq %0,%%fs:%P1(,%q2,8)" : \ > >>> + : "r" (value), \ > >>> + "i" (offsetof (struct pthread, member[0])), \ > >>> + "r" (idx)); \ > >>> else /* 8 */ \ > >>> { \ > >>> asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ > >>> > >> > > > > Here is the v2 patch. OK for master? > > > > Thanks. > > > > > From 4fbc9ab67933d662506a01658d4c81a922e50fdf Mon Sep 17 00:00:00 2001 > > From: "H.J. Lu" <hjl.tools@gmail.com> > > Date: Fri, 8 Jan 2021 15:38:14 -0800 > > Subject: [PATCH v2] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 > > > > Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use > > "movq reg64, mem64" to store 64-bit constant. > > --- > > sysdeps/x86_64/nptl/tls.h | 27 ++++++++++++++++++++------- > > 1 file changed, 20 insertions(+), 7 deletions(-) > > > > diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h > > index 20f0958780..0dbd66209c 100644 > > --- a/sysdeps/x86_64/nptl/tls.h > > +++ b/sysdeps/x86_64/nptl/tls.h > > @@ -271,9 +271,15 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, > > "i" (offsetof (struct pthread, member))); \ > > else /* 8 */ \ > > { \ > > - asm volatile ("movq %q0,%%fs:%P1" : \ > > - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > > - "i" (offsetof (struct pthread, member))); \ > > + if (!__builtin_constant_p (value) \ > > Whis is this a "!__builtin_constant_p?" > > I would have expected: > > if ([the value is a constant a therefore known quantity] > || [it is a 32-bit value]) IMM_MODE constraint is valid only if VALUE isn't a constant or is a signed 32-bit constant. > Perhaps a quick single line comment here explaining the conditionals would help? Done. Here is the v3 patch with comments. OK for master? Thanks. -- H.J. [-- Attachment #2: v3-0001-x86_64-Update-THREAD_SETMEM-THREAD_SETMEM_NC-for-.patch --] [-- Type: text/x-patch, Size: 2675 bytes --] From 0a5d266206be231ab11f3cfa0e213db1e04cd7bd Mon Sep 17 00:00:00 2001 From: "H.J. Lu" <hjl.tools@gmail.com> Date: Fri, 8 Jan 2021 15:38:14 -0800 Subject: [PATCH v3] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use "movq reg64, mem64" to store 64-bit constant. --- sysdeps/x86_64/nptl/tls.h | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h index 20f0958780..acea951d49 100644 --- a/sysdeps/x86_64/nptl/tls.h +++ b/sysdeps/x86_64/nptl/tls.h @@ -271,9 +271,17 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, "i" (offsetof (struct pthread, member))); \ else /* 8 */ \ { \ - asm volatile ("movq %q0,%%fs:%P1" : \ - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ - "i" (offsetof (struct pthread, member))); \ + /* NB: IMM_MODE constraint is valid only if VALUE isn't a constant \ + or is a signed 32-bit constant. */ \ + if (!__builtin_constant_p (value) \ + || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ + asm volatile ("movq %q0,%%fs:%P1" : \ + : IMM_MODE ((uint64_t) cast_to_integer (value)), \ + "i" (offsetof (struct pthread, member))); \ + else \ + asm volatile ("movq %0,%%fs:%P1" : \ + : "r" (value), \ + "i" (offsetof (struct pthread, member))); \ }}) @@ -296,10 +304,19 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, "r" (idx)); \ else /* 8 */ \ { \ - asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ - "i" (offsetof (struct pthread, member[0])), \ - "r" (idx)); \ + /* NB: IMM_MODE constraint is valid only if VALUE isn't a constant \ + or is a signed 32-bit constant. */ \ + if (!__builtin_constant_p (value) \ + || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ + asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ + : IMM_MODE ((uint64_t) cast_to_integer (value)), \ + "i" (offsetof (struct pthread, member[0])), \ + "r" (idx)); \ + else \ + asm volatile ("movq %0,%%fs:%P1(,%q2,8)" : \ + : "r" (value), \ + "i" (offsetof (struct pthread, member[0])), \ + "r" (idx)); \ }}) -- 2.29.2 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 2021-03-09 0:09 ` [PATCH v3] " H.J. Lu @ 2021-03-15 12:49 ` Carlos O'Donell 2021-03-15 13:29 ` [PATCH v4] " H.J. Lu 0 siblings, 1 reply; 15+ messages in thread From: Carlos O'Donell @ 2021-03-15 12:49 UTC (permalink / raw) To: H.J. Lu; +Cc: GNU C Library On 3/8/21 7:09 PM, H.J. Lu wrote: > On Mon, Mar 8, 2021 at 2:28 PM Carlos O'Donell <carlos@redhat.com> wrote: >> >> On 3/2/21 9:21 AM, H.J. Lu wrote: >>> On Mon, Mar 1, 2021 at 5:30 AM Carlos O'Donell <carlos@redhat.com> wrote: >>>> >>>> On 2/2/21 2:12 PM, H.J. Lu via Libc-alpha wrote: >>>>> Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use >>>>> "movq reg64, mem64" to store 64-bit constant in TCB. >>>>> --- >>>>> sysdeps/x86_64/nptl/tls.h | 11 +++++++++++ >>>>> 1 file changed, 11 insertions(+) >>>>> >>>>> diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h >>>>> index 20f0958780..9ec8703e45 100644 >>>>> --- a/sysdeps/x86_64/nptl/tls.h >>>>> +++ b/sysdeps/x86_64/nptl/tls.h >>>>> @@ -269,6 +269,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, >>>>> asm volatile ("movl %0,%%fs:%P1" : \ >>>>> : IMM_MODE (value), \ >>>>> "i" (offsetof (struct pthread, member))); \ >>>>> + else if (__builtin_constant_p (value) \ >>>>> + && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value) \ >>>>> + asm volatile ("movq %0,%%fs:%P1" : \ >>>>> + : "r" (value), \ >>>>> + "i" (offsetof (struct pthread, member))); \ >>>> >>>> (1) Move conditional into 'else /* 8 */' section. >>>> >>>> The blocks of conditionals are predicated on the size of the member we are >>>> about to write to. >>>> >>>> In the block below... >>>> >>>>> else /* 8 */ \ >>>> >>>> ... here, we are about to write value into a member that is size 8. >>>> >>>> Your code changes the logical construction of the code, but in way that makes >>>> it more difficult to understand. >>>> >>>> We previously had: >>>> >>>> if (sizeof() == 1) >>>> >>>> else if (sizeof() == 4) >>>> >>>> else /* Assume 8 */ >>>> >>>> In your case we must already be in the 'else /* Assume 8 */' because otherwise >>>> we've be writing a 64-bit constant into a < 64-bit structure member. >>>> >>>> I think we should put your code into the else clause. >>>> >>>> 272 else /* 8 */ \ >>>> 273 { \ >>>> >>>> if (__builtin_constant_p (value) >>>> && ([the value is a >32-bit constant]) >>>> asm volatile ([use movq reg64, mem64]); >>>> else >>>> >>>> 274 asm volatile ("movq %q0,%%fs:%P1" : \ >>>> 275 : IMM_MODE ((uint64_t) cast_to_integer (value)), \ >>>> 276 "i" (offsetof (struct pthread, member))); \ >>>> 277 }}) >>>> >>>> (2) What if gcc can't prove it's constant? >>>> >>>> If the constant is >32-bit, but gcc can't prove it's constant, then don't we >>>> try to put a >32-bit constant into a 32-bit immediate? >>>> >>>> Shouldn't the code be structured the other way around? >>>> >>>> e.g. >>>> >>>> else /* 8 */ >>>> { >>>> if (__builtin_constant_p (value) >>>> && ([the value is a <= 32-bit constant]) >>>> asm volatile ([use movq imm32, mem64]); >>>> else >>>> asm volatile ([use movq reg64, mem64]); >>>> } >>>> >>>> This way the code is always correct? >>> >>> I changed it to >>> >>> if (!__builtin_constant_p (value) \ >>> || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ >>> asm volatile ("movq %q0,%%fs:%P1" : \ >>> : IMM_MODE ((uint64_t) cast_to_integer (value)), \ >>> "i" (offsetof (struct pthread, member))); \ >>> else \ >>> asm volatile ("movq %0,%%fs:%P1" : \ >>> : "r" (value), \ >>> "i" (offsetof (struct pthread, member))); \ >>> >>>>> { \ >>>>> asm volatile ("movq %q0,%%fs:%P1" : \ >>>>> @@ -294,6 +299,12 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, >>>>> : IMM_MODE (value), \ >>>>> "i" (offsetof (struct pthread, member[0])), \ >>>>> "r" (idx)); \ >>>>> + else if (__builtin_constant_p (value) \ >>>>> + && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value) \ >>>>> + asm volatile ("movq %0,%%fs:%P1(,%q2,8)" : \ >>>>> + : "r" (value), \ >>>>> + "i" (offsetof (struct pthread, member[0])), \ >>>>> + "r" (idx)); \ >>>>> else /* 8 */ \ >>>>> { \ >>>>> asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ >>>>> >>>> >>> >>> Here is the v2 patch. OK for master? >>> >>> Thanks. >>> >> >>> From 4fbc9ab67933d662506a01658d4c81a922e50fdf Mon Sep 17 00:00:00 2001 >>> From: "H.J. Lu" <hjl.tools@gmail.com> >>> Date: Fri, 8 Jan 2021 15:38:14 -0800 >>> Subject: [PATCH v2] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 >>> >>> Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use >>> "movq reg64, mem64" to store 64-bit constant. >>> --- >>> sysdeps/x86_64/nptl/tls.h | 27 ++++++++++++++++++++------- >>> 1 file changed, 20 insertions(+), 7 deletions(-) >>> >>> diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h >>> index 20f0958780..0dbd66209c 100644 >>> --- a/sysdeps/x86_64/nptl/tls.h >>> +++ b/sysdeps/x86_64/nptl/tls.h >>> @@ -271,9 +271,15 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, >>> "i" (offsetof (struct pthread, member))); \ >>> else /* 8 */ \ >>> { \ >>> - asm volatile ("movq %q0,%%fs:%P1" : \ >>> - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ >>> - "i" (offsetof (struct pthread, member))); \ >>> + if (!__builtin_constant_p (value) \ >> >> Whis is this a "!__builtin_constant_p?" >> >> I would have expected: >> >> if ([the value is a constant a therefore known quantity] >> || [it is a 32-bit value]) > > IMM_MODE constraint is valid only if VALUE isn't a constant or > is a signed 32-bit constant. > >> Perhaps a quick single line comment here explaining the conditionals would help? > > Done. > > Here is the v3 patch with comments. OK for master? No, this still doesn't make logical sense to me. See my comments below. > From 0a5d266206be231ab11f3cfa0e213db1e04cd7bd Mon Sep 17 00:00:00 2001 > From: "H.J. Lu" <hjl.tools@gmail.com> > Date: Fri, 8 Jan 2021 15:38:14 -0800 > Subject: [PATCH v3] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 > > Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use > "movq reg64, mem64" to store 64-bit constant. > --- > sysdeps/x86_64/nptl/tls.h | 31 ++++++++++++++++++++++++------- > 1 file changed, 24 insertions(+), 7 deletions(-) > > diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h > index 20f0958780..acea951d49 100644 > --- a/sysdeps/x86_64/nptl/tls.h > +++ b/sysdeps/x86_64/nptl/tls.h > @@ -271,9 +271,17 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, > "i" (offsetof (struct pthread, member))); \ > else /* 8 */ \ > { \ > - asm volatile ("movq %q0,%%fs:%P1" : \ > - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > - "i" (offsetof (struct pthread, member))); \ > + /* NB: IMM_MODE constraint is valid only if VALUE isn't a constant \ > + or is a signed 32-bit constant. */ \ We are trying to gate this asm volatile so it is only used by the compiler when the immediate is known to be 32-bit wide. The old code was using IMM_MODE, which resolves to "nr" which should as a constraint attempt to pass in a compile-time constant 32-bit value. My understanding is that this constraint could fail to be met and the we get a compile-time failure? The value of !__builtin_constant_p only tells you that the value is not a constant. What prevents that value from being a 64-bit register with a 64-bit value which can't be encoded via 'nr' with movq? Why isn't the right solution? if (__builtin_constant_p (value) && (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)) /* The value is constant and known to be <= 32-bit immediate value. In this case we allow the compiler to optimize and use movq with an immediate value. */ ... else /* The value is not known and so we must assume it is at worst a 64-bit value. We can't optimize this and so we must use a register for the move. */ ... > + if (!__builtin_constant_p (value) \ > + || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ > + asm volatile ("movq %q0,%%fs:%P1" : \ > + : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > + "i" (offsetof (struct pthread, member))); \ > + else \ > + asm volatile ("movq %0,%%fs:%P1" : \ > + : "r" (value), \ > + "i" (offsetof (struct pthread, member))); \ > }}) > > > @@ -296,10 +304,19 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, > "r" (idx)); \ > else /* 8 */ \ > { \ > - asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ > - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > - "i" (offsetof (struct pthread, member[0])), \ > - "r" (idx)); \ > + /* NB: IMM_MODE constraint is valid only if VALUE isn't a constant \ > + or is a signed 32-bit constant. */ \ > + if (!__builtin_constant_p (value) \ > + || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ > + asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ > + : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > + "i" (offsetof (struct pthread, member[0])), \ > + "r" (idx)); \ > + else \ > + asm volatile ("movq %0,%%fs:%P1(,%q2,8)" : \ > + : "r" (value), \ > + "i" (offsetof (struct pthread, member[0])), \ > + "r" (idx)); \ > }}) > > > -- > 2.29.2 > -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 2021-03-15 12:49 ` Carlos O'Donell @ 2021-03-15 13:29 ` H.J. Lu 2021-03-16 3:01 ` Carlos O'Donell 0 siblings, 1 reply; 15+ messages in thread From: H.J. Lu @ 2021-03-15 13:29 UTC (permalink / raw) To: Carlos O'Donell; +Cc: GNU C Library [-- Attachment #1: Type: text/plain, Size: 10538 bytes --] On Mon, Mar 15, 2021 at 5:49 AM Carlos O'Donell <carlos@redhat.com> wrote: > > On 3/8/21 7:09 PM, H.J. Lu wrote: > > On Mon, Mar 8, 2021 at 2:28 PM Carlos O'Donell <carlos@redhat.com> wrote: > >> > >> On 3/2/21 9:21 AM, H.J. Lu wrote: > >>> On Mon, Mar 1, 2021 at 5:30 AM Carlos O'Donell <carlos@redhat.com> wrote: > >>>> > >>>> On 2/2/21 2:12 PM, H.J. Lu via Libc-alpha wrote: > >>>>> Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use > >>>>> "movq reg64, mem64" to store 64-bit constant in TCB. > >>>>> --- > >>>>> sysdeps/x86_64/nptl/tls.h | 11 +++++++++++ > >>>>> 1 file changed, 11 insertions(+) > >>>>> > >>>>> diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h > >>>>> index 20f0958780..9ec8703e45 100644 > >>>>> --- a/sysdeps/x86_64/nptl/tls.h > >>>>> +++ b/sysdeps/x86_64/nptl/tls.h > >>>>> @@ -269,6 +269,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, > >>>>> asm volatile ("movl %0,%%fs:%P1" : \ > >>>>> : IMM_MODE (value), \ > >>>>> "i" (offsetof (struct pthread, member))); \ > >>>>> + else if (__builtin_constant_p (value) \ > >>>>> + && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value) \ > >>>>> + asm volatile ("movq %0,%%fs:%P1" : \ > >>>>> + : "r" (value), \ > >>>>> + "i" (offsetof (struct pthread, member))); \ > >>>> > >>>> (1) Move conditional into 'else /* 8 */' section. > >>>> > >>>> The blocks of conditionals are predicated on the size of the member we are > >>>> about to write to. > >>>> > >>>> In the block below... > >>>> > >>>>> else /* 8 */ \ > >>>> > >>>> ... here, we are about to write value into a member that is size 8. > >>>> > >>>> Your code changes the logical construction of the code, but in way that makes > >>>> it more difficult to understand. > >>>> > >>>> We previously had: > >>>> > >>>> if (sizeof() == 1) > >>>> > >>>> else if (sizeof() == 4) > >>>> > >>>> else /* Assume 8 */ > >>>> > >>>> In your case we must already be in the 'else /* Assume 8 */' because otherwise > >>>> we've be writing a 64-bit constant into a < 64-bit structure member. > >>>> > >>>> I think we should put your code into the else clause. > >>>> > >>>> 272 else /* 8 */ \ > >>>> 273 { \ > >>>> > >>>> if (__builtin_constant_p (value) > >>>> && ([the value is a >32-bit constant]) > >>>> asm volatile ([use movq reg64, mem64]); > >>>> else > >>>> > >>>> 274 asm volatile ("movq %q0,%%fs:%P1" : \ > >>>> 275 : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > >>>> 276 "i" (offsetof (struct pthread, member))); \ > >>>> 277 }}) > >>>> > >>>> (2) What if gcc can't prove it's constant? > >>>> > >>>> If the constant is >32-bit, but gcc can't prove it's constant, then don't we > >>>> try to put a >32-bit constant into a 32-bit immediate? > >>>> > >>>> Shouldn't the code be structured the other way around? > >>>> > >>>> e.g. > >>>> > >>>> else /* 8 */ > >>>> { > >>>> if (__builtin_constant_p (value) > >>>> && ([the value is a <= 32-bit constant]) > >>>> asm volatile ([use movq imm32, mem64]); > >>>> else > >>>> asm volatile ([use movq reg64, mem64]); > >>>> } > >>>> > >>>> This way the code is always correct? > >>> > >>> I changed it to > >>> > >>> if (!__builtin_constant_p (value) \ > >>> || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ > >>> asm volatile ("movq %q0,%%fs:%P1" : \ > >>> : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > >>> "i" (offsetof (struct pthread, member))); \ > >>> else \ > >>> asm volatile ("movq %0,%%fs:%P1" : \ > >>> : "r" (value), \ > >>> "i" (offsetof (struct pthread, member))); \ > >>> > >>>>> { \ > >>>>> asm volatile ("movq %q0,%%fs:%P1" : \ > >>>>> @@ -294,6 +299,12 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, > >>>>> : IMM_MODE (value), \ > >>>>> "i" (offsetof (struct pthread, member[0])), \ > >>>>> "r" (idx)); \ > >>>>> + else if (__builtin_constant_p (value) \ > >>>>> + && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value) \ > >>>>> + asm volatile ("movq %0,%%fs:%P1(,%q2,8)" : \ > >>>>> + : "r" (value), \ > >>>>> + "i" (offsetof (struct pthread, member[0])), \ > >>>>> + "r" (idx)); \ > >>>>> else /* 8 */ \ > >>>>> { \ > >>>>> asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ > >>>>> > >>>> > >>> > >>> Here is the v2 patch. OK for master? > >>> > >>> Thanks. > >>> > >> > >>> From 4fbc9ab67933d662506a01658d4c81a922e50fdf Mon Sep 17 00:00:00 2001 > >>> From: "H.J. Lu" <hjl.tools@gmail.com> > >>> Date: Fri, 8 Jan 2021 15:38:14 -0800 > >>> Subject: [PATCH v2] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 > >>> > >>> Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use > >>> "movq reg64, mem64" to store 64-bit constant. > >>> --- > >>> sysdeps/x86_64/nptl/tls.h | 27 ++++++++++++++++++++------- > >>> 1 file changed, 20 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h > >>> index 20f0958780..0dbd66209c 100644 > >>> --- a/sysdeps/x86_64/nptl/tls.h > >>> +++ b/sysdeps/x86_64/nptl/tls.h > >>> @@ -271,9 +271,15 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, > >>> "i" (offsetof (struct pthread, member))); \ > >>> else /* 8 */ \ > >>> { \ > >>> - asm volatile ("movq %q0,%%fs:%P1" : \ > >>> - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > >>> - "i" (offsetof (struct pthread, member))); \ > >>> + if (!__builtin_constant_p (value) \ > >> > >> Whis is this a "!__builtin_constant_p?" > >> > >> I would have expected: > >> > >> if ([the value is a constant a therefore known quantity] > >> || [it is a 32-bit value]) > > > > IMM_MODE constraint is valid only if VALUE isn't a constant or > > is a signed 32-bit constant. > > > >> Perhaps a quick single line comment here explaining the conditionals would help? > > > > Done. > > > > Here is the v3 patch with comments. OK for master? > > No, this still doesn't make logical sense to me. > > See my comments below. > > > From 0a5d266206be231ab11f3cfa0e213db1e04cd7bd Mon Sep 17 00:00:00 2001 > > From: "H.J. Lu" <hjl.tools@gmail.com> > > Date: Fri, 8 Jan 2021 15:38:14 -0800 > > Subject: [PATCH v3] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 > > > > Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use > > "movq reg64, mem64" to store 64-bit constant. > > --- > > sysdeps/x86_64/nptl/tls.h | 31 ++++++++++++++++++++++++------- > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h > > index 20f0958780..acea951d49 100644 > > --- a/sysdeps/x86_64/nptl/tls.h > > +++ b/sysdeps/x86_64/nptl/tls.h > > @@ -271,9 +271,17 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, > > "i" (offsetof (struct pthread, member))); \ > > else /* 8 */ \ > > { \ > > - asm volatile ("movq %q0,%%fs:%P1" : \ > > - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > > - "i" (offsetof (struct pthread, member))); \ > > + /* NB: IMM_MODE constraint is valid only if VALUE isn't a constant \ > > + or is a signed 32-bit constant. */ \ > > We are trying to gate this asm volatile so it is only used by the compiler > when the immediate is known to be 32-bit wide. > > The old code was using IMM_MODE, which resolves to "nr" which should as a > constraint attempt to pass in a compile-time constant 32-bit value. My > understanding is that this constraint could fail to be met and the we get > a compile-time failure? > > The value of !__builtin_constant_p only tells you that the value is not a > constant. What prevents that value from being a 64-bit register with a 64-bit > value which can't be encoded via 'nr' with movq? > > Why isn't the right solution? > > if (__builtin_constant_p (value) > && (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)) > /* The value is constant and known to be <= 32-bit immediate value. > In this case we allow the compiler to optimize and use movq with > an immediate value. */ > ... > else > /* The value is not known and so we must assume it is at worst a 64-bit value. > We can't optimize this and so we must use a register for the move. */ > ... > Fixed. Here is the v4 patch. OK for master? Thanks. -- H.J. [-- Attachment #2: v4-0001-x86_64-Update-THREAD_SETMEM-THREAD_SETMEM_NC-for-.patch --] [-- Type: text/x-patch, Size: 2851 bytes --] From d3854da28e1cfc2544226629b3b85ad6e2d6f4d4 Mon Sep 17 00:00:00 2001 From: "H.J. Lu" <hjl.tools@gmail.com> Date: Fri, 8 Jan 2021 15:38:14 -0800 Subject: [PATCH v4] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use "movq reg64, mem64" to store 64-bit constant. --- sysdeps/x86_64/nptl/tls.h | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h index 20f0958780..2a4739df64 100644 --- a/sysdeps/x86_64/nptl/tls.h +++ b/sysdeps/x86_64/nptl/tls.h @@ -271,9 +271,18 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, "i" (offsetof (struct pthread, member))); \ else /* 8 */ \ { \ - asm volatile ("movq %q0,%%fs:%P1" : \ - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ - "i" (offsetof (struct pthread, member))); \ + /* The value is constant and known to be <= 32-bit immediate value. \ + In this case we allow the compiler to optimize and use movq with \ + an immediate value. */ \ + if (__builtin_constant_p (value) \ + && (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ + asm volatile ("movq %q0,%%fs:%P1" : \ + : "i" ((uint64_t) cast_to_integer (value)), \ + "i" (offsetof (struct pthread, member))); \ + else \ + asm volatile ("movq %0,%%fs:%P1" : \ + : IMM_MODE ((uint64_t) cast_to_integer (value)), \ + "i" (offsetof (struct pthread, member))); \ }}) @@ -296,10 +305,20 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, "r" (idx)); \ else /* 8 */ \ { \ - asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ - "i" (offsetof (struct pthread, member[0])), \ - "r" (idx)); \ + /* The value is constant and known to be <= 32-bit immediate value. \ + In this case we allow the compiler to optimize and use movq with \ + an immediate value. */ \ + if (__builtin_constant_p (value) \ + && (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ + asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ + : "i" ((uint64_t) cast_to_integer (value)), \ + "i" (offsetof (struct pthread, member[0])), \ + "r" (idx)); \ + else \ + asm volatile ("movq %0,%%fs:%P1(,%q2,8)" : \ + : IMM_MODE ((uint64_t) cast_to_integer (value)), \ + "i" (offsetof (struct pthread, member[0])), \ + "r" (idx)); \ }}) -- 2.30.2 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 2021-03-15 13:29 ` [PATCH v4] " H.J. Lu @ 2021-03-16 3:01 ` Carlos O'Donell 2021-03-16 9:04 ` Andreas Schwab 0 siblings, 1 reply; 15+ messages in thread From: Carlos O'Donell @ 2021-03-16 3:01 UTC (permalink / raw) To: H.J. Lu; +Cc: GNU C Library On 3/15/21 9:29 AM, H.J. Lu wrote: > Here is the v4 patch. OK for master? Let me start from the beginning, just to clarify a few things for my review. The original sequence was this: asm volatile ("movq %0,%%fs:%P1" : : IMM_MODE ((uint64_t) cast_to_integer (value)), "i" (offsetof (struct pthread, member))); With IMM_MODE being 'nr'/'ir' and allows: i/n - immediate integer operand r - register operand You attempt to write a 64-bit constant to the TCB and the above code fails. Does gcc attempt to encode a 64-bit immediate into %0 and the assembler fails? What was the error? Then we try to fix this with the new sequence: asm volatile ("movq %q0,%%fs:%P1" : : "i" ((uint64_t) cast_to_integer (value)), "i" (offsetof (struct pthread, member))); Note that "%q0" is a machine constraint for any register. This enforces only "i" (immediate integer operand) but requires the compiler pick a register with the machine constraint for "%q0", so you can have 64-bit constants but the compiler must arrange (via more machine instructions to get the constant in into a 64-bit register). Your v4 follows: > From d3854da28e1cfc2544226629b3b85ad6e2d6f4d4 Mon Sep 17 00:00:00 2001 > From: "H.J. Lu" <hjl.tools@gmail.com> > Date: Fri, 8 Jan 2021 15:38:14 -0800 > Subject: [PATCH v4] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 > > Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use > "movq reg64, mem64" to store 64-bit constant> --- > sysdeps/x86_64/nptl/tls.h | 33 ++++++++++++++++++++++++++------- > 1 file changed, 26 insertions(+), 7 deletions(-) > > diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h > index 20f0958780..2a4739df64 100644 > --- a/sysdeps/x86_64/nptl/tls.h > +++ b/sysdeps/x86_64/nptl/tls.h > @@ -271,9 +271,18 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, > "i" (offsetof (struct pthread, member))); \ > else /* 8 */ \ > { \ > - asm volatile ("movq %q0,%%fs:%P1" : \ > - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > - "i" (offsetof (struct pthread, member))); \ > + /* The value is constant and known to be <= 32-bit immediate value. \ > + In this case we allow the compiler to optimize and use movq with \ > + an immediate value. */ \ > + if (__builtin_constant_p (value) \ > + && (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ Note that I made a typo on my last review. My intent was to write this: /* The value is constant and known to be <= 32-bit immediate value. \ In this case we allow the compiler to optimize and use movq with \ an immediate value. */ \ if (__builtin_constant_p (value) && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value)) asm volatile ("movq %0,%%fs:%P1" : \ : IMM_MODE ((uint64_t) cast_to_integer (value)), \ "i" (offsetof (struct pthread, member))); \ else /* The value is not known and so we must assume it is at worst a 64-bit value. We can't optimize this and so we must use a register for the move. */ asm volatile ("movq %q0,%%fs:%P1" : \ : "i" ((uint64_t) cast_to_integer (value)), \ "i" (offsetof (struct pthread, member))); \ So in the case you write a 64-bit constant you get the else case. But in the case you have normal known 32-bit values you get the movq with a imm32s. Does that work? Can you please verify that you get the first case for all the normal 32-bit constants? Can you please verify that you get the second for the 64-bit constant write? > + asm volatile ("movq %q0,%%fs:%P1" : \ > + : "i" ((uint64_t) cast_to_integer (value)), \ > + "i" (offsetof (struct pthread, member))); \ > + else \ > + asm volatile ("movq %0,%%fs:%P1" : \ > + : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > + "i" (offsetof (struct pthread, member))); \ > }}) > > > @@ -296,10 +305,20 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, > "r" (idx)); \ > else /* 8 */ \ > { \ > - asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ > - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > - "i" (offsetof (struct pthread, member[0])), \ > - "r" (idx)); \ > + /* The value is constant and known to be <= 32-bit immediate value. \ > + In this case we allow the compiler to optimize and use movq with \ > + an immediate value. */ \ > + if (__builtin_constant_p (value) \ > + && (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ > + asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ > + : "i" ((uint64_t) cast_to_integer (value)), \ > + "i" (offsetof (struct pthread, member[0])), \ > + "r" (idx)); \ > + else \ > + asm volatile ("movq %0,%%fs:%P1(,%q2,8)" : \ > + : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > + "i" (offsetof (struct pthread, member[0])), \ > + "r" (idx)); \ > }}) > > > -- > 2.30.2 > -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 2021-03-16 3:01 ` Carlos O'Donell @ 2021-03-16 9:04 ` Andreas Schwab 2021-03-16 15:45 ` [PATCH v5] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 [BZ #27591] H.J. Lu 0 siblings, 1 reply; 15+ messages in thread From: Andreas Schwab @ 2021-03-16 9:04 UTC (permalink / raw) To: Carlos O'Donell via Libc-alpha On Mär 15 2021, Carlos O'Donell via Libc-alpha wrote: > Then we try to fix this with the new sequence: > > asm volatile ("movq %q0,%%fs:%P1" : > : "i" ((uint64_t) cast_to_integer (value)), > "i" (offsetof (struct pthread, member))); > > Note that "%q0" is a machine constraint for any register. An operand modifier isn't a constraint. It does not change what the compiler matches for the operand, it only modifies the way the operand is output. 'q' has no meaning for an operand matched by the 'i' constraint, since that is never a register. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different." ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v5] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 [BZ #27591] 2021-03-16 9:04 ` Andreas Schwab @ 2021-03-16 15:45 ` H.J. Lu 2021-03-16 16:02 ` Andreas Schwab 0 siblings, 1 reply; 15+ messages in thread From: H.J. Lu @ 2021-03-16 15:45 UTC (permalink / raw) To: Andreas Schwab; +Cc: Carlos O'Donell via Libc-alpha, Carlos O'Donell [-- Attachment #1: Type: text/plain, Size: 982 bytes --] On Tue, Mar 16, 2021 at 2:04 AM Andreas Schwab <schwab@linux-m68k.org> wrote: > > On Mär 15 2021, Carlos O'Donell via Libc-alpha wrote: > > > Then we try to fix this with the new sequence: > > > > asm volatile ("movq %q0,%%fs:%P1" : > > : "i" ((uint64_t) cast_to_integer (value)), > > "i" (offsetof (struct pthread, member))); > > > > Note that "%q0" is a machine constraint for any register. > > An operand modifier isn't a constraint. It does not change what the > compiler matches for the operand, it only modifies the way the operand > is output. 'q' has no meaning for an operand matched by the 'i' > constraint, since that is never a register. That is correct. Since movq takes a register or a signed 32-bit immediate source operand, select the immediate source operand only if it is known to be 32-bit signed immediate at compile-time. Here is the v5 patch with a testcase. OK for master? Thanks. -- H.J. [-- Attachment #2: v5-0001-x86_64-Update-THREAD_SETMEM-THREAD_SETMEM_NC-for-.patch --] [-- Type: text/x-patch, Size: 5673 bytes --] From 199f147a2c95549890fcb1c8d4c3598bce45545b Mon Sep 17 00:00:00 2001 From: "H.J. Lu" <hjl.tools@gmail.com> Date: Tue, 16 Mar 2021 07:41:46 -0700 Subject: [PATCH v5] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 [BZ #27591] Since movq takes a register or a signed 32-bit immediate source operand, select the immediate source operand only if it is known to be 32-bit signed immediate at compile-time. --- sysdeps/x86_64/Makefile | 2 ++ sysdeps/x86_64/nptl/tls.h | 33 +++++++++++++++----- sysdeps/x86_64/tst-x86-64-tls-1.c | 50 +++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 7 deletions(-) create mode 100644 sysdeps/x86_64/tst-x86-64-tls-1.c diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile index d1d7cb9d2e..06a444b6af 100644 --- a/sysdeps/x86_64/Makefile +++ b/sysdeps/x86_64/Makefile @@ -183,6 +183,8 @@ ifeq (no,$(build-hardcoded-path-in-tests)) tests-container += tst-glibc-hwcaps-cache endif +tests-internal += tst-x86-64-tls-1 + endif # $(subdir) == elf ifeq ($(subdir),csu) diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h index 20f0958780..b19d90433d 100644 --- a/sysdeps/x86_64/nptl/tls.h +++ b/sysdeps/x86_64/nptl/tls.h @@ -271,9 +271,18 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, "i" (offsetof (struct pthread, member))); \ else /* 8 */ \ { \ - asm volatile ("movq %q0,%%fs:%P1" : \ - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ - "i" (offsetof (struct pthread, member))); \ + /* Since movq takes a register or a signed 32-bit immediate source \ + operand, select the immediate source operand only if it is known \ + to be 32-bit signed immediate at compile-time. */ \ + if (__builtin_constant_p (value) \ + && (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ + asm volatile ("movq %0,%%fs:%P1" : \ + : "i" ((uint64_t) cast_to_integer (value)), \ + "i" (offsetof (struct pthread, member))); \ + else \ + asm volatile ("movq %q0,%%fs:%P1" : \ + : "r" ((uint64_t) cast_to_integer (value)), \ + "i" (offsetof (struct pthread, member))); \ }}) @@ -296,10 +305,20 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, "r" (idx)); \ else /* 8 */ \ { \ - asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ - "i" (offsetof (struct pthread, member[0])), \ - "r" (idx)); \ + /* Since movq takes a register or a signed 32-bit immediate source \ + operand, select the immediate source operand only if it is known \ + to be 32-bit signed immediate at compile-time. */ \ + if (__builtin_constant_p (value) \ + && (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ + asm volatile ("movq %0,%%fs:%P1(,%q2,8)" : \ + : "i" ((uint64_t) cast_to_integer (value)), \ + "i" (offsetof (struct pthread, member[0])), \ + "r" (idx)); \ + else \ + asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ + : "r" ((uint64_t) cast_to_integer (value)), \ + "i" (offsetof (struct pthread, member[0])), \ + "r" (idx)); \ }}) diff --git a/sysdeps/x86_64/tst-x86-64-tls-1.c b/sysdeps/x86_64/tst-x86-64-tls-1.c new file mode 100644 index 0000000000..ac6b13d3b7 --- /dev/null +++ b/sysdeps/x86_64/tst-x86-64-tls-1.c @@ -0,0 +1,50 @@ +/* Test THREAD_SETMEM and THREAD_SETMEM_NC for IMM64. + Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <tls.h> +#include <support/check.h> + +static int +do_test (void) +{ + unsigned long long int saved_ssp_base + = THREAD_GETMEM (THREAD_SELF, header.ssp_base); + THREAD_SETMEM (THREAD_SELF, header.ssp_base, (1ULL << 57) - 1); + unsigned long long int ssp_base + = THREAD_GETMEM (THREAD_SELF, header.ssp_base); + if (ssp_base != ((1ULL << 57) - 1)) + FAIL_EXIT1 ("THREAD_SETMEM: 0x%llx != 0x%llx", + ssp_base, (1ULL << 57) - 1); + THREAD_SETMEM (THREAD_SELF, header.ssp_base, saved_ssp_base); +#ifndef __ILP32__ + struct pthread_key_data *saved_specific + = THREAD_GETMEM_NC (THREAD_SELF, specific, 1); + uintptr_t value = (1UL << 57) - 1; + THREAD_SETMEM_NC (THREAD_SELF, specific, 1, + (struct pthread_key_data *) value); + struct pthread_key_data *specific + = THREAD_GETMEM_NC (THREAD_SELF, specific, 1); + if (specific != (struct pthread_key_data *) value) + FAIL_EXIT1 ("THREAD_GETMEM_NC: %p != %p", + specific, (struct pthread_key_data *) value); + THREAD_SETMEM_NC (THREAD_SELF, specific, 1, saved_specific); +#endif + return 0; +} + +#include <support/test-driver.c> -- 2.30.2 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 [BZ #27591] 2021-03-16 15:45 ` [PATCH v5] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 [BZ #27591] H.J. Lu @ 2021-03-16 16:02 ` Andreas Schwab 2021-03-16 16:10 ` Florian Weimer 0 siblings, 1 reply; 15+ messages in thread From: Andreas Schwab @ 2021-03-16 16:02 UTC (permalink / raw) To: H.J. Lu; +Cc: Carlos O'Donell via Libc-alpha, Carlos O'Donell On Mär 16 2021, H.J. Lu wrote: > diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h > index 20f0958780..b19d90433d 100644 > --- a/sysdeps/x86_64/nptl/tls.h > +++ b/sysdeps/x86_64/nptl/tls.h > @@ -271,9 +271,18 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, > "i" (offsetof (struct pthread, member))); \ > else /* 8 */ \ > { \ > - asm volatile ("movq %q0,%%fs:%P1" : \ > - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ > - "i" (offsetof (struct pthread, member))); \ > + /* Since movq takes a register or a signed 32-bit immediate source \ > + operand, select the immediate source operand only if it is known \ > + to be 32-bit signed immediate at compile-time. */ \ > + if (__builtin_constant_p (value) \ > + && (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value) \ > + asm volatile ("movq %0,%%fs:%P1" : \ > + : "i" ((uint64_t) cast_to_integer (value)), \ > + "i" (offsetof (struct pthread, member))); \ > + else \ > + asm volatile ("movq %q0,%%fs:%P1" : \ > + : "r" ((uint64_t) cast_to_integer (value)), \ > + "i" (offsetof (struct pthread, member))); \ I don't think this will work in general. Wouldn't it be possible that an operands optimizes to a 64-bit constant in later passes? What you really need is a constraint that only matches a 32-bit immediate or a register. Like "rWf"? Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different." ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 [BZ #27591] 2021-03-16 16:02 ` Andreas Schwab @ 2021-03-16 16:10 ` Florian Weimer 2021-03-16 16:34 ` Carlos O'Donell 0 siblings, 1 reply; 15+ messages in thread From: Florian Weimer @ 2021-03-16 16:10 UTC (permalink / raw) To: Andreas Schwab; +Cc: H.J. Lu, Carlos O'Donell via Libc-alpha * Andreas Schwab: > I don't think this will work in general. Wouldn't it be possible that > an operands optimizes to a 64-bit constant in later passes? What you > really need is a constraint that only matches a 32-bit immediate or a > register. Like "rWf"? Maybe we should to the __seg_fs namespace instead? Wouldn't that avoid these issues? Thanks, Florian ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 [BZ #27591] 2021-03-16 16:10 ` Florian Weimer @ 2021-03-16 16:34 ` Carlos O'Donell 2021-03-16 16:50 ` [PATCH v6] x86_64: Correct THREAD_SETMEM/THREAD_SETMEM_NC for movq " H.J. Lu 0 siblings, 1 reply; 15+ messages in thread From: Carlos O'Donell @ 2021-03-16 16:34 UTC (permalink / raw) To: Florian Weimer, Andreas Schwab, H.J. Lu Cc: Carlos O'Donell via Libc-alpha On 3/16/21 12:10 PM, Florian Weimer via Libc-alpha wrote: > * Andreas Schwab: > >> I don't think this will work in general. Wouldn't it be possible that >> an operands optimizes to a 64-bit constant in later passes? What you >> really need is a constraint that only matches a 32-bit immediate or a >> register. Like "rWf"? > > Maybe we should to the __seg_fs namespace instead? Wouldn't that avoid > these issues? It might. HJ, Does it work to rewrite this using __seg_fs? Otherwise I'll review v5, which I think is better than it was before and makes forward progress. -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v6] x86_64: Correct THREAD_SETMEM/THREAD_SETMEM_NC for movq [BZ #27591] 2021-03-16 16:34 ` Carlos O'Donell @ 2021-03-16 16:50 ` H.J. Lu 2021-03-16 16:52 ` H.J. Lu 0 siblings, 1 reply; 15+ messages in thread From: H.J. Lu @ 2021-03-16 16:50 UTC (permalink / raw) To: Carlos O'Donell Cc: Florian Weimer, Andreas Schwab, Carlos O'Donell via Libc-alpha On Tue, Mar 16, 2021 at 9:34 AM Carlos O'Donell <carlos@redhat.com> wrote: > > On 3/16/21 12:10 PM, Florian Weimer via Libc-alpha wrote: > > * Andreas Schwab: > > > >> I don't think this will work in general. Wouldn't it be possible that > >> an operands optimizes to a 64-bit constant in later passes? What you > >> really need is a constraint that only matches a 32-bit immediate or a > >> register. Like "rWf"? config/i386/constraints.md in GCC has (define_constraint "e" "32-bit signed integer constant, or a symbolic reference known to fit that range (for immediate operands in sign-extending x86-64 instructions)." (match_operand 0 "x86_64_immediate_operand")) "er" is the correct constraint for movq. > > Maybe we should to the __seg_fs namespace instead? Wouldn't that avoid > > these issues? > > It might. > > HJ, Does it work to rewrite this using __seg_fs? I tried it. GCC generates worse code. > Otherwise I'll review v5, which I think is better than it was before and > makes forward progress. Here is the v6 patch to use the "er" constraint. OK for master? Thanks. -- H.J. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v6] x86_64: Correct THREAD_SETMEM/THREAD_SETMEM_NC for movq [BZ #27591] 2021-03-16 16:50 ` [PATCH v6] x86_64: Correct THREAD_SETMEM/THREAD_SETMEM_NC for movq " H.J. Lu @ 2021-03-16 16:52 ` H.J. Lu 0 siblings, 0 replies; 15+ messages in thread From: H.J. Lu @ 2021-03-16 16:52 UTC (permalink / raw) To: Carlos O'Donell Cc: Florian Weimer, Andreas Schwab, Carlos O'Donell via Libc-alpha [-- Attachment #1: Type: text/plain, Size: 1257 bytes --] On Tue, Mar 16, 2021 at 9:50 AM H.J. Lu <hjl.tools@gmail.com> wrote: > > On Tue, Mar 16, 2021 at 9:34 AM Carlos O'Donell <carlos@redhat.com> wrote: > > > > On 3/16/21 12:10 PM, Florian Weimer via Libc-alpha wrote: > > > * Andreas Schwab: > > > > > >> I don't think this will work in general. Wouldn't it be possible that > > >> an operands optimizes to a 64-bit constant in later passes? What you > > >> really need is a constraint that only matches a 32-bit immediate or a > > >> register. Like "rWf"? > > config/i386/constraints.md in GCC has > > (define_constraint "e" > "32-bit signed integer constant, or a symbolic reference known > to fit that range (for immediate operands in sign-extending x86-64 > instructions)." > (match_operand 0 "x86_64_immediate_operand")) > > "er" is the correct constraint for movq. > > > > Maybe we should to the __seg_fs namespace instead? Wouldn't that avoid > > > these issues? > > > > It might. > > > > HJ, Does it work to rewrite this using __seg_fs? > > I tried it. GCC generates worse code. > > > Otherwise I'll review v5, which I think is better than it was before and > > makes forward progress. > > Here is the v6 patch to use the "er" constraint. OK for master? > Include the patch. -- H.J. [-- Attachment #2: v6-0001-x86_64-Correct-THREAD_SETMEM-THREAD_SETMEM_NC-for.patch --] [-- Type: text/x-patch, Size: 5458 bytes --] From 98ff24c0c7fba29c90ac37edd0057515aacdafa0 Mon Sep 17 00:00:00 2001 From: "H.J. Lu" <hjl.tools@gmail.com> Date: Tue, 16 Mar 2021 07:41:46 -0700 Subject: [PATCH v6] x86_64: Correct THREAD_SETMEM/THREAD_SETMEM_NC for movq [BZ #27591] config/i386/constraints.md in GCC has (define_constraint "e" "32-bit signed integer constant, or a symbolic reference known to fit that range (for immediate operands in sign-extending x86-64 instructions)." (match_operand 0 "x86_64_immediate_operand")) Since movq takes a signed 32-bit immediate or a register source operand, use "er", instead of "nr"/"ir", constraint for 32-bit signed integer constant or register on movq. --- sysdeps/x86_64/Makefile | 2 + sysdeps/x86_64/nptl/tls.h | 10 ++++- sysdeps/x86_64/tst-x86-64-tls-1.c | 64 +++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 sysdeps/x86_64/tst-x86-64-tls-1.c diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile index d1d7cb9d2e..06a444b6af 100644 --- a/sysdeps/x86_64/Makefile +++ b/sysdeps/x86_64/Makefile @@ -183,6 +183,8 @@ ifeq (no,$(build-hardcoded-path-in-tests)) tests-container += tst-glibc-hwcaps-cache endif +tests-internal += tst-x86-64-tls-1 + endif # $(subdir) == elf ifeq ($(subdir),csu) diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h index 20f0958780..a78c4f4d01 100644 --- a/sysdeps/x86_64/nptl/tls.h +++ b/sysdeps/x86_64/nptl/tls.h @@ -271,8 +271,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, "i" (offsetof (struct pthread, member))); \ else /* 8 */ \ { \ + /* Since movq takes a signed 32-bit immediate or a register source \ + operand, use "er" constraint for 32-bit signed integer constant \ + or register. */ \ asm volatile ("movq %q0,%%fs:%P1" : \ - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ + : "er" ((uint64_t) cast_to_integer (value)), \ "i" (offsetof (struct pthread, member))); \ }}) @@ -296,8 +299,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80, "r" (idx)); \ else /* 8 */ \ { \ + /* Since movq takes a signed 32-bit immediate or a register source \ + operand, use "er" constraint for 32-bit signed integer constant \ + or register. */ \ asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" : \ - : IMM_MODE ((uint64_t) cast_to_integer (value)), \ + : "er" ((uint64_t) cast_to_integer (value)), \ "i" (offsetof (struct pthread, member[0])), \ "r" (idx)); \ }}) diff --git a/sysdeps/x86_64/tst-x86-64-tls-1.c b/sysdeps/x86_64/tst-x86-64-tls-1.c new file mode 100644 index 0000000000..354635884e --- /dev/null +++ b/sysdeps/x86_64/tst-x86-64-tls-1.c @@ -0,0 +1,64 @@ +/* Test THREAD_SETMEM and THREAD_SETMEM_NC for IMM64. + Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <tls.h> +#include <support/check.h> + +static int +do_test (void) +{ + unsigned long long int saved_ssp_base, ssp_base; + saved_ssp_base = THREAD_GETMEM (THREAD_SELF, header.ssp_base); + + THREAD_SETMEM (THREAD_SELF, header.ssp_base, (1ULL << 57) - 1); + ssp_base = THREAD_GETMEM (THREAD_SELF, header.ssp_base); + if (ssp_base != ((1ULL << 57) - 1)) + FAIL_EXIT1 ("THREAD_SETMEM: 0x%llx != 0x%llx", + ssp_base, (1ULL << 57) - 1); + + THREAD_SETMEM (THREAD_SELF, header.ssp_base, -1ULL); + ssp_base = THREAD_GETMEM (THREAD_SELF, header.ssp_base); + if (ssp_base != -1ULL) + FAIL_EXIT1 ("THREAD_SETMEM: 0x%llx != 0x%llx", ssp_base, -1ULL); + + THREAD_SETMEM (THREAD_SELF, header.ssp_base, saved_ssp_base); +#ifndef __ILP32__ + struct pthread_key_data *saved_specific, *specific; + saved_specific = THREAD_GETMEM_NC (THREAD_SELF, specific, 1); + + uintptr_t value = (1UL << 57) - 1; + THREAD_SETMEM_NC (THREAD_SELF, specific, 1, + (struct pthread_key_data *) value); + specific = THREAD_GETMEM_NC (THREAD_SELF, specific, 1); + if (specific != (struct pthread_key_data *) value) + FAIL_EXIT1 ("THREAD_GETMEM_NC: %p != %p", + specific, (struct pthread_key_data *) value); + + THREAD_SETMEM_NC (THREAD_SELF, specific, 1, + (struct pthread_key_data *) -1UL); + specific = THREAD_GETMEM_NC (THREAD_SELF, specific, 1); + if (specific != (struct pthread_key_data *) -1UL) + FAIL_EXIT1 ("THREAD_GETMEM_NC: %p != %p", + specific, (struct pthread_key_data *) -1UL); + + THREAD_SETMEM_NC (THREAD_SELF, specific, 1, saved_specific); +#endif + return 0; +} + +#include <support/test-driver.c> -- 2.30.2 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-03-16 16:53 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-02 19:12 [PATCH] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 H.J. Lu 2021-03-01 13:30 ` Carlos O'Donell 2021-03-02 14:21 ` [PATCH v2] " H.J. Lu 2021-03-08 22:28 ` Carlos O'Donell 2021-03-09 0:09 ` [PATCH v3] " H.J. Lu 2021-03-15 12:49 ` Carlos O'Donell 2021-03-15 13:29 ` [PATCH v4] " H.J. Lu 2021-03-16 3:01 ` Carlos O'Donell 2021-03-16 9:04 ` Andreas Schwab 2021-03-16 15:45 ` [PATCH v5] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 [BZ #27591] H.J. Lu 2021-03-16 16:02 ` Andreas Schwab 2021-03-16 16:10 ` Florian Weimer 2021-03-16 16:34 ` Carlos O'Donell 2021-03-16 16:50 ` [PATCH v6] x86_64: Correct THREAD_SETMEM/THREAD_SETMEM_NC for movq " H.J. Lu 2021-03-16 16:52 ` H.J. Lu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).