Re: [PATCH v4] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64

[Carlos O'Donell <carlos@redhat.com>]

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.