Message ID | CAMe9rOrXqn2=00FT4Rw2Eq7q9y_uCXYZ=nxd3LMOtSv9RJNg+Q@mail.gmail.com |
---|---|
State | New |
Headers | show |
Series | [v3] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 | expand |
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 >
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