From patchwork Tue Mar 9 00:09:52 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "H.J. Lu" X-Patchwork-Id: 42392 X-Patchwork-Delegate: carlos@redhat.com Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A06973851C29; Tue, 9 Mar 2021 00:10:32 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org A06973851C29 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1615248632; bh=Op5KSrRyKiS/Tr+4GxI/+uG3AmbupB/GqhJdOt/G3LY=; h=References:In-Reply-To:Date:Subject:To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=b4vY2SII8Qbwkp2dyJoARn8X0lDdNepIInU6x6T0MxqelL22GZeHusJD+Uyp0LGIx a6hwbPio3fxQA6I7CRYLZWGKCN8/GqVXu7K7nabv6PRup1b2PaCEZbtxCy9Tln+QoS EH8yUP21Yo44c9hyeRJF1aS75Gv8b2b8x5ki6n5Y= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-ot1-x336.google.com (mail-ot1-x336.google.com [IPv6:2607:f8b0:4864:20::336]) by sourceware.org (Postfix) with ESMTPS id C00853857828 for ; Tue, 9 Mar 2021 00:10:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C00853857828 Received: by mail-ot1-x336.google.com with SMTP id h22so11112085otr.6 for ; Mon, 08 Mar 2021 16:10:29 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Op5KSrRyKiS/Tr+4GxI/+uG3AmbupB/GqhJdOt/G3LY=; b=TK9zwClYLjvUZdOtR3nU54+NR7uPupHbDVljL+ZnU7MIvnZ5BNaWGlaygRFh/pjGKV AWZ5LRKsoDho4wJiOmgE6fz8i04W4UBJeeoVS2KlHf0C7eNAwbugWotk3o21ZqM5SrhA cYAG3fgAosDg1pekusRpWpFkoJzMw07ALR+WoM87j+r/WvRkpYrhs7WckQlYNAk9Mu22 mW5xsc7JlVTT9uBA4TKATT1t3wosNGny/biVPCWnD6g/tfhbRnnC20cSk3Fd8L3hYloW p8g0w+WPwzP1r1rZz/Bfgk1l3nMT/9mNh0tgOWVrb5meItPHmSwqWmmYCyuTlHS2ieHL OTZg== X-Gm-Message-State: AOAM533d16fk3dLOT+YZH8V22CWUyorgzasfyB2DbqdpTJx5WcKdoyoA IhQJKAlzwCh3fFVp8PtzCPg3XJcUVhpjydHD/nJiQtUJfNM= X-Google-Smtp-Source: ABdhPJz3SYqhfMMzUWg9mRJmPcJdc2us7/DW8geXSagWMyKv87vc38kcAEsse9YKPSqExW4F2Gwvf8AaR4EuX9T5GGc= X-Received: by 2002:a05:6830:1515:: with SMTP id k21mr2922550otp.269.1615248629089; Mon, 08 Mar 2021 16:10:29 -0800 (PST) MIME-Version: 1.0 References: <20210202191209.4036619-1-hjl.tools@gmail.com> <60a3a304-7f33-3727-a39a-5420d26d13a0@redhat.com> In-Reply-To: <60a3a304-7f33-3727-a39a-5420d26d13a0@redhat.com> Date: Mon, 8 Mar 2021 16:09:52 -0800 Message-ID: Subject: [PATCH v3] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 To: "Carlos O'Donell" X-Spam-Status: No, score=-3035.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_NUMSUBJECT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: "H.J. Lu via Libc-alpha" From: "H.J. Lu" Reply-To: "H.J. Lu" Cc: GNU C Library Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" On Mon, Mar 8, 2021 at 2:28 PM Carlos O'Donell wrote: > > On 3/2/21 9:21 AM, H.J. Lu wrote: > > On Mon, Mar 1, 2021 at 5:30 AM Carlos O'Donell 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" > > 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. From 0a5d266206be231ab11f3cfa0e213db1e04cd7bd Mon Sep 17 00:00:00 2001 From: "H.J. Lu" 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