Message ID | 20221109190225.96037-2-aldot@gcc.gnu.org |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> 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 6889F384406B for <patchwork@sourceware.org>; Wed, 9 Nov 2022 19:03:19 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 6889F384406B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1668020599; bh=JniFKryGt7U/WHxlDgYxNiM9IVmvLlQiEQJuByNMa60=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=nOFSA8H8NDNNgkK0Z/ulHXkyTcS+bqZnN+2D03ZQOjalt3ZRrSr8RoECfiLA3uyOS Q7n56q1SZBelzn98J8zBf9nJx9o+yAilrYhG7Ro+YOvmm432H31GmoFqyfrXtRv6dX w4gIiX9Se6LTpA74DXftN5vyADoUkPfFd3tNjCAM= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-ej1-x635.google.com (mail-ej1-x635.google.com [IPv6:2a00:1450:4864:20::635]) by sourceware.org (Postfix) with ESMTPS id CC0793858D28; Wed, 9 Nov 2022 19:02:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CC0793858D28 Received: by mail-ej1-x635.google.com with SMTP id ft34so13060765ejc.12; Wed, 09 Nov 2022 11:02:39 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=JniFKryGt7U/WHxlDgYxNiM9IVmvLlQiEQJuByNMa60=; b=khpd7FlN/6bsp8mjVPETIZY4gjx2AK+3gAz5uwGP41h5muSKNMFIJ8AAo/eTp8FTT1 vB4MA0b5zWM457FO876yBg8DMK99xih2VCOz+/b7eBdG2488aQ6fCne8isEMguEYtglE TMFPk0Xw1J6AgNAxrb7gnD5sfYMHf98NRF1ChoSPIiANsoJxXVvzYdBOpJxrfeyfH1FX 1Sz8UjG4r4vSW1ypAoiz/OMsfYgVh760cNB/qLvHYPgvhTT9Skh2POCv7sXUBXV3NQIb ywH7QoGt03iH/Q0xkDf49gj9ZHGctNHxb8HB/pXqODowtHEHkv8sBG1c+LUcOHii/9p4 XyPg== X-Gm-Message-State: ACrzQf3d6lrGrfY8V81jTlsDvPYQHkm5elZ0pNLiFPbU7MW0CbyQ4m6d cEalfyg563eCAoV2vl4YL/0= X-Google-Smtp-Source: AMsMyM64fNbv7f+/iqMoAbUBxZyNDlIMZP2gHoVskHbpRq2ekkYERothdX8VdIOzDMPMh/aNbwjiew== X-Received: by 2002:a17:906:cc4e:b0:7ae:3f78:c8b8 with SMTP id mm14-20020a170906cc4e00b007ae3f78c8b8mr24895593ejb.394.1668020558702; Wed, 09 Nov 2022 11:02:38 -0800 (PST) Received: from nbbrfq (80-110-214-113.static.upcbusiness.at. [80.110.214.113]) by smtp.gmail.com with ESMTPSA id i28-20020a1709067a5c00b0077f20a722dfsm6123674ejo.165.2022.11.09.11.02.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 09 Nov 2022 11:02:37 -0800 (PST) X-Google-Original-From: Bernhard Reutner-Fischer <aldot@gcc.gnu.org> Received: from b by nbbrfq with local (Exim 4.96) (envelope-from <b@localhost>) id 1osqLM-000OzL-1z; Wed, 09 Nov 2022 20:02:36 +0100 To: gcc-patches@gcc.gnu.org Cc: Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>, Bernhard Reutner-Fischer <aldot@gcc.gnu.org>, gfortran ML <fortran@gcc.gnu.org>, Jan Hubicka <hubicka@ucw.cz> Subject: [PATCH 1/2] symtab: also change RTL decl name Date: Wed, 9 Nov 2022 20:02:24 +0100 Message-Id: <20221109190225.96037-2-aldot@gcc.gnu.org> X-Mailer: git-send-email 2.38.1 In-Reply-To: <20221109190225.96037-1-aldot@gcc.gnu.org> References: <20221109190225.96037-1-aldot@gcc.gnu.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Bernhard Reutner-Fischer via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
Fortran: add attribute target_clones
|
|
Commit Message
Bernhard Reutner-Fischer
Nov. 9, 2022, 7:02 p.m. UTC
We were changing the ASSEMBLER_NAME of the function decl but not the name in DECL_RTL which is used as the function name fnname in rest_of_handle_final(). This led to using the old, wrong name for the attribute target default function when using target_clones. Bootstrapped and regtested cleanly on x86_64-unknown-linux for c,c++,fortran,lto. Ok for trunk? gcc/ChangeLog: * symtab.cc: Remove stray comment. (symbol_table::change_decl_assembler_name): Also update the name in DECL_RTL. Cc: Jan Hubicka <hubicka@ucw.cz> --- gcc/symtab.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Comments
Hi Honza, Ping. Regtests cleanly for c,fortran,c++,ada,d,go,lto,objc,obj-c++ Ok? I'd need this for attribute target_clones for the Fortran FE. thanks, On Wed, 9 Nov 2022 20:02:24 +0100 Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: > We were changing the ASSEMBLER_NAME of the function decl > but not the name in DECL_RTL which is used as the function name > fnname in rest_of_handle_final(). This led to using the old, wrong name > for the attribute target default function when using target_clones. > > Bootstrapped and regtested cleanly on x86_64-unknown-linux > for c,c++,fortran,lto. > Ok for trunk? > > gcc/ChangeLog: > > * symtab.cc: Remove stray comment. > (symbol_table::change_decl_assembler_name): Also update the > name in DECL_RTL. > > Cc: Jan Hubicka <hubicka@ucw.cz> > --- > gcc/symtab.cc | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/gcc/symtab.cc b/gcc/symtab.cc > index f2d96c0268b..2e20bf5fefc 100644 > --- a/gcc/symtab.cc > +++ b/gcc/symtab.cc > @@ -154,8 +154,6 @@ symbol_table::decl_assembler_name_equal (tree decl, const_tree asmname) > } > > > -/* Returns nonzero if P1 and P2 are equal. */ > - > /* Insert NODE to assembler name hash. */ > > void > @@ -303,6 +301,10 @@ symbol_table::change_decl_assembler_name (tree decl, tree name) > warning (0, "%qD renamed after being referenced in assembly", decl); > > SET_DECL_ASSEMBLER_NAME (decl, name); > + /* Set the new name in rtl. */ > + if (DECL_RTL_SET_P (decl)) > + XSTR (XEXP (DECL_RTL (decl), 0), 0) = IDENTIFIER_POINTER (name); > + > if (alias) > { > IDENTIFIER_TRANSPARENT_ALIAS (name) = 1;
Hello, Le 17/11/2022 à 09:02, Bernhard Reutner-Fischer via Fortran a écrit : > Hi Honza, Ping. > Regtests cleanly for c,fortran,c++,ada,d,go,lto,objc,obj-c++ > Ok? > I'd need this for attribute target_clones for the Fortran FE. > thanks, > > On Wed, 9 Nov 2022 20:02:24 +0100 > Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: > >> We were changing the ASSEMBLER_NAME of the function decl >> but not the name in DECL_RTL which is used as the function name >> fnname in rest_of_handle_final(). This led to using the old, wrong name >> for the attribute target default function when using target_clones. >> >> Bootstrapped and regtested cleanly on x86_64-unknown-linux >> for c,c++,fortran,lto. >> Ok for trunk? >> >> gcc/ChangeLog: >> >> * symtab.cc: Remove stray comment. >> (symbol_table::change_decl_assembler_name): Also update the >> name in DECL_RTL. >> (patch stripped) Is there a PR about this? Or a testcase exhibiting the problem at least?
> Hi Honza, Ping. > Regtests cleanly for c,fortran,c++,ada,d,go,lto,objc,obj-c++ > Ok? > I'd need this for attribute target_clones for the Fortran FE. Sorry for delay here. > > void > > @@ -303,6 +301,10 @@ symbol_table::change_decl_assembler_name (tree decl, tree name) > > warning (0, "%qD renamed after being referenced in assembly", decl); > > > > SET_DECL_ASSEMBLER_NAME (decl, name); > > + /* Set the new name in rtl. */ > > + if (DECL_RTL_SET_P (decl)) > > + XSTR (XEXP (DECL_RTL (decl), 0), 0) = IDENTIFIER_POINTER (name); I am not quite sure how safe this is. We generally produce DECL_RTL when we produce assembly file. So if DECL_RTL is set then we probably already output the original function name and it is too late to change it. Also RTL is shared so changing it in-place is going to rewrite all the existing RTL expressions using it. Why the DECL_RTL is produced for function you want to rename? Honza > > + > > if (alias) > > { > > IDENTIFIER_TRANSPARENT_ALIAS (name) = 1; >
On Mon, 21 Nov 2022 20:02:49 +0100 Jan Hubicka <hubicka@ucw.cz> wrote: > > Hi Honza, Ping. > > Regtests cleanly for c,fortran,c++,ada,d,go,lto,objc,obj-c++ > > Ok? > > I'd need this for attribute target_clones for the Fortran FE. > Sorry for delay here. > > > void > > > @@ -303,6 +301,10 @@ symbol_table::change_decl_assembler_name (tree decl, tree name) > > > warning (0, "%qD renamed after being referenced in assembly", decl); > > > > > > SET_DECL_ASSEMBLER_NAME (decl, name); > > > + /* Set the new name in rtl. */ > > > + if (DECL_RTL_SET_P (decl)) > > > + XSTR (XEXP (DECL_RTL (decl), 0), 0) = IDENTIFIER_POINTER (name); > > I am not quite sure how safe this is. We generally produce DECL_RTL > when we produce assembly file. So if DECL_RTL is set then we probably > already output the original function name and it is too late to change > it. AFAICS we make_decl_rtl in the fortran FE in trans_function_start. > > Also RTL is shared so changing it in-place is going to rewrite all the > existing RTL expressions using it. > > Why the DECL_RTL is produced for function you want to rename? I think the fortran FE sets it quite early when lowering a function. Later, when the ME creates the target_clones, it wants to rename the initial function to initial_fun.default for the default target. That's where the change_decl_assembler_name is called (only on the decl). But nobody changes the RTL name, so the ifunc (which should be the initial, unchanged name) is properly emitted but assemble_start_function uses the same, unchanged, initial fnname it later obtains by get_fnname_from_decl which fetches the (wrong) initial name where it should use the .default target name. See https://gcc.gnu.org/pipermail/gcc-patches/2022-November/605081.html I'm open to other suggestions to make this work in a different way, of course. Maybe we're missing some magic somewhere that might share the name between the fndecl and the RTL XSTR so the RTL is magically updated by that single SET_ECL_ASSEMBLER_NAME in change_decl_assembler_name? But i didn't quite see where that'd be? thanks, > Honza > > > + > > > if (alias) > > > { > > > IDENTIFIER_TRANSPARENT_ALIAS (name) = 1; > >
> On Mon, 21 Nov 2022 20:02:49 +0100 > Jan Hubicka <hubicka@ucw.cz> wrote: > > > > Hi Honza, Ping. > > > Regtests cleanly for c,fortran,c++,ada,d,go,lto,objc,obj-c++ > > > Ok? > > > I'd need this for attribute target_clones for the Fortran FE. > > Sorry for delay here. > > > > void > > > > @@ -303,6 +301,10 @@ symbol_table::change_decl_assembler_name (tree decl, tree name) > > > > warning (0, "%qD renamed after being referenced in assembly", decl); > > > > > > > > SET_DECL_ASSEMBLER_NAME (decl, name); > > > > + /* Set the new name in rtl. */ > > > > + if (DECL_RTL_SET_P (decl)) > > > > + XSTR (XEXP (DECL_RTL (decl), 0), 0) = IDENTIFIER_POINTER (name); > > > > I am not quite sure how safe this is. We generally produce DECL_RTL > > when we produce assembly file. So if DECL_RTL is set then we probably > > already output the original function name and it is too late to change > > it. > > AFAICS we make_decl_rtl in the fortran FE in trans_function_start. I see, it may be a relic of something that is no longer necessary. Can you see why one needs DECL_RTL so early? > > > > > Also RTL is shared so changing it in-place is going to rewrite all the > > existing RTL expressions using it. > > > > Why the DECL_RTL is produced for function you want to rename? > > I think the fortran FE sets it quite early when lowering a function. > Later, when the ME creates the target_clones, it wants to rename the > initial function to initial_fun.default for the default target. > That's where the change_decl_assembler_name is called (only on the > decl). > But nobody changes the RTL name, so the ifunc (which should be the > initial, unchanged name) is properly emitted but > assemble_start_function uses the same, unchanged, initial fnname it > later obtains by get_fnname_from_decl which fetches the (wrong) initial > name where it should use the .default target name. > See > https://gcc.gnu.org/pipermail/gcc-patches/2022-November/605081.html > > I'm open to other suggestions to make this work in a different way, of > course. Maybe we're missing some magic somewhere that might share the > name between the fndecl and the RTL XSTR so the RTL is magically > updated by that single SET_ECL_ASSEMBLER_NAME in > change_decl_assembler_name? But i didn't quite see where that'd be? I think we should start by understanding why Fortran FE produces DECL_RTL early. It was written before symbol table code emerged, it may be simply an oversight I made while converting FE to symbol table. Honza > > thanks, > > > Honza > > > > + > > > > if (alias) > > > > { > > > > IDENTIFIER_TRANSPARENT_ALIAS (name) = 1; > > > >
On Tue, 22 Nov 2022 12:54:01 +0100 Jan Hubicka <hubicka@ucw.cz> wrote: > > > I am not quite sure how safe this is. We generally produce DECL_RTL > > > when we produce assembly file. So if DECL_RTL is set then we probably > > > already output the original function name and it is too late to change > > > it. > > > > AFAICS we make_decl_rtl in the fortran FE in trans_function_start. > > I see, it may be a relic of something that is no longer necessary. Can > you see why one needs DECL_RTL so early? No. I think this is a ward, isn't it. diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc index ff64588b9a8..a801e66fb11 100644 --- a/gcc/fortran/trans-decl.cc +++ b/gcc/fortran/trans-decl.cc @@ -2922,7 +2922,7 @@ trans_function_start (gfc_symbol * sym) } /* Create RTL for function definition. */ - make_decl_rtl (fndecl); + //make_decl_rtl (fndecl); allocate_struct_function (fndecl, false); But we have that alot, with varous workarounds near lhd_set_decl_assembler_name. gcc.gnu.org/pr94223 comes to mind, but that was counters. But i've seen in C++ too that there are GC dangles like here. 5f3682ffcef162363b783eb9ee702debff489fa8 a.k.a https://gcc.gnu.org/legacy-ml/gcc-patches/2017-11/msg01340.html ah lhd_overwrite_decl_assembler_name. So.. why do we have that, again? Per default it doesn't look much if there are clones or an ifunc dispatcher, does it.
diff --git a/gcc/symtab.cc b/gcc/symtab.cc index f2d96c0268b..2e20bf5fefc 100644 --- a/gcc/symtab.cc +++ b/gcc/symtab.cc @@ -154,8 +154,6 @@ symbol_table::decl_assembler_name_equal (tree decl, const_tree asmname) } -/* Returns nonzero if P1 and P2 are equal. */ - /* Insert NODE to assembler name hash. */ void @@ -303,6 +301,10 @@ symbol_table::change_decl_assembler_name (tree decl, tree name) warning (0, "%qD renamed after being referenced in assembly", decl); SET_DECL_ASSEMBLER_NAME (decl, name); + /* Set the new name in rtl. */ + if (DECL_RTL_SET_P (decl)) + XSTR (XEXP (DECL_RTL (decl), 0), 0) = IDENTIFIER_POINTER (name); + if (alias) { IDENTIFIER_TRANSPARENT_ALIAS (name) = 1;