From patchwork Tue Mar 2 14:21:00 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: 42198 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 002B23836C4C; Tue, 2 Mar 2021 14:21:39 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 002B23836C4C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1614694900; bh=LcNKSQoENbRK1EYlizriUQHiFhWhg8BQGn7EqsTAGFQ=; 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=rWrAtxiPvQcz4B4Dr3aSjMOT5B+/lxXyW5gTkUmqHrXgRo/i9d6iMdJvuAKLTiSBr jdpS4SQzLEAsID/7Hy4IcwgxRbMluRRLwb/Aed7hpWHGQKsHpnzEGrV4FNC/VXLonC 9SQX7ZPl5KTkeKsveT9Uyb7vuTv9uac4fTLcyRYc= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-oi1-x236.google.com (mail-oi1-x236.google.com [IPv6:2607:f8b0:4864:20::236]) by sourceware.org (Postfix) with ESMTPS id E214D3848027 for ; Tue, 2 Mar 2021 14:21:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E214D3848027 Received: by mail-oi1-x236.google.com with SMTP id m25so9078901oie.12 for ; Tue, 02 Mar 2021 06:21:36 -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=LcNKSQoENbRK1EYlizriUQHiFhWhg8BQGn7EqsTAGFQ=; b=qfzNYW9SKQV+FI9riyMhNiSMgl18ekp+n3kiD1588O/2NyJwmMq+7c1gO9WZVoU6xy wQvmdFFgKi/Z5UBMUjIS+9B8UQMrhp2ioWOkbcmW5QKoEf0nKsJWwD0YBrI++D75vQlB +R7bItUqUWuQHVeeGpvIL378lm+WhAPhIoGIHB/GDBTU8+jhliwNprDl8JvNcmWsNVE6 jbqS6T94zFUGLHnhQl+9RdY1pU1qkcwplhysPb0OS98xeCZH9OohMooYDiFihzXiGyya 5jsBZE9EuJwLp7XM9cMpwjqQx+ony7gD6RuyO8o0cOF8+qaPh4sZjHYfpzYw9pbz0/b/ hpQw== X-Gm-Message-State: AOAM532jYips1RVqBmnYzona/mEa2NULJWWXrFh5kQjv/TIbqWKUt8Kw 45WsTTlOqkvmd5zboTKZKPvQwu02TCedwHTc+WmgaPtzsiE= X-Google-Smtp-Source: ABdhPJySHOUDr/i9NzBWpPUqIvAX1TyjY6wAVbgbgGFO/ByVuk7coZSHvwkxCFrRBSk3/5YRfkQM2dkRqiR0ll62kPA= X-Received: by 2002:aca:5783:: with SMTP id l125mr3389679oib.79.1614694896296; Tue, 02 Mar 2021 06:21:36 -0800 (PST) MIME-Version: 1.0 References: <20210202191209.4036619-1-hjl.tools@gmail.com> In-Reply-To: Date: Tue, 2 Mar 2021 06:21:00 -0800 Message-ID: Subject: [PATCH v2] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 To: "Carlos O'Donell" X-Spam-Status: No, score=-3034.7 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 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) \ + || (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