[v5] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 [BZ #27591]

  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.
  

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:

> 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
  

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.
  

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.
  

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.