From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 46971 invoked by alias); 20 Jan 2020 11:47:24 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 46922 invoked by uid 89); 20 Jan 2020 11:47:23 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_NUMSUBJECT,KAM_STOCKGEN,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=ILP32 X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.110.172) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 20 Jan 2020 11:47:13 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id F307430E; Mon, 20 Jan 2020 03:47:11 -0800 (PST) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.126]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7F9CF3F68E; Mon, 20 Jan 2020 03:47:11 -0800 (PST) From: Richard Sandiford To: Mail-Followup-To: ,, richard.sandiford@arm.com Cc: Subject: Re: [PATCH] Fix target/93119 (aarch64): ICE with traditional TLS support on ILP32 References: <1579302817-19505-1-git-send-email-apinski@marvell.com> Date: Mon, 20 Jan 2020 12:16:00 -0000 In-Reply-To: <1579302817-19505-1-git-send-email-apinski@marvell.com> (apinski's message of "Fri, 17 Jan 2020 15:13:37 -0800") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2020-01/txt/msg01201.txt.bz2 writes: > From: Andrew Pinski > > The problem here was g:23b88fda665d2f995c was not a complete fix > for supporting tranditional TLS on ILP32. > > So the problem here is a couple of things, first __tls_get_addr > call will return a C pointer value so we need to use ptr_mode > when we are creating the call. Then we need to convert > back that register to the correct mode, either zero extending > it or just creating a move instruction. This and the comment: > + /* Convert back to the mode of the dest, adding a zero-extend as > + needed. */ > + if (dest != tmp_reg) > + convert_move (dest, tmp_reg, true); make it sound like the convert_move is handling two separate cases. But AFAICT the convert_move only ever zero-extends, it should never emit just a plain move. Is that right? > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index c26ac0db942..6c825459dc6 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -2607,19 +2607,29 @@ aarch64_load_symref_appropriately (rtx dest, rtx imm, > case SYMBOL_SMALL_TLSGD: > { > rtx_insn *insns; > - machine_mode mode = GET_MODE (dest); > - rtx result = gen_rtx_REG (mode, R0_REGNUM); > + /* The return type of __tls_get_addr is the C pointer type > + so use ptr_mode. */ > + rtx result = gen_rtx_REG (ptr_mode, R0_REGNUM); > + rtx tmp_reg = dest; > + > + if (GET_MODE (dest) != ptr_mode) > + tmp_reg = can_create_pseudo_p () ? gen_reg_rtx (ptr_mode) : result; > > start_sequence (); > - if (TARGET_ILP32) > + if (ptr_mode == SImode) > aarch64_emit_call_insn (gen_tlsgd_small_si (result, imm)); > else > aarch64_emit_call_insn (gen_tlsgd_small_di (result, imm)); > + > insns = get_insns (); > end_sequence (); I think this looks more obvious without the extra blank line, so that the whole start_sequence/end_sequence is a single block. If the convert_move does always zero-extend, the patch is OK without the extra blank line and with an updated comment. Thanks, Richard > > RTL_CONST_CALL_P (insns) = 1; > - emit_libcall_block (insns, dest, result, imm); > + emit_libcall_block (insns, tmp_reg, result, imm); > + /* Convert back to the mode of the dest, adding a zero-extend as > + needed. */ > + if (dest != tmp_reg) > + convert_move (dest, tmp_reg, true); > return; > } > > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > index 86c2cdfc797..55dde54b16a 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -6755,10 +6755,10 @@ (define_insn "aarch64_load_tp_hard" > ;; instructions in the TLS stubs, in order to enable linker relaxation. > ;; Therefore we treat the stubs as an atomic sequence. > (define_expand "tlsgd_small_" > - [(parallel [(set (match_operand 0 "register_operand") > + [(parallel [(set (match_operand:PTR 0 "register_operand") > (call (mem:DI (match_dup 2)) (const_int 1))) > (unspec:DI [(const_int 0)] UNSPEC_CALLEE_ABI) > - (unspec:DI [(match_operand:PTR 1 "aarch64_valid_symref")] UNSPEC_GOTSMALLTLS) > + (unspec:DI [(match_operand 1 "aarch64_valid_symref")] UNSPEC_GOTSMALLTLS) > (clobber (reg:DI LR_REGNUM))])] > "" > { > @@ -6766,10 +6766,10 @@ (define_expand "tlsgd_small_" > }) > > (define_insn "*tlsgd_small_" > - [(set (match_operand 0 "register_operand" "") > + [(set (match_operand:PTR 0 "register_operand" "") > (call (mem:DI (match_operand:DI 2 "" "")) (const_int 1))) > (unspec:DI [(const_int 0)] UNSPEC_CALLEE_ABI) > - (unspec:DI [(match_operand:PTR 1 "aarch64_valid_symref" "S")] UNSPEC_GOTSMALLTLS) > + (unspec:DI [(match_operand 1 "aarch64_valid_symref" "S")] UNSPEC_GOTSMALLTLS) > (clobber (reg:DI LR_REGNUM)) > ] > "" > diff --git a/gcc/testsuite/gcc.target/aarch64/pr93119.c b/gcc/testsuite/gcc.target/aarch64/pr93119.c > new file mode 100644 > index 00000000000..93fa80e10b6 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/pr93119.c > @@ -0,0 +1,10 @@ > +/* { dg-require-effective-target fpic } */ > +/* { dg-options "-mtls-dialect=trad -fpic" } */ > + > +__thread int g_tlsdata; > + > +int func1() > +{ > + g_tlsdata++; > + return g_tlsdata; > +}