From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26969 invoked by alias); 15 Oct 2014 14:36:05 -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 26957 invoked by uid 89); 15 Oct 2014 14:36:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.0 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mail2-relais-roc.national.inria.fr Received: from mail2-relais-roc.national.inria.fr (HELO mail2-relais-roc.national.inria.fr) (192.134.164.83) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Wed, 15 Oct 2014 14:36:03 +0000 Received: from stedding.saclay.inria.fr (HELO stedding) ([193.55.250.194]) by mail2-relais-roc.national.inria.fr with ESMTP/TLS/AES128-SHA; 15 Oct 2014 16:35:59 +0200 Received: from glisse (helo=localhost) by stedding with local-esmtp (Exim 4.84) (envelope-from ) id 1XePgE-0003mp-W4 for gcc-patches@gcc.gnu.org; Wed, 15 Oct 2014 16:35:59 +0200 Date: Wed, 15 Oct 2014 14:36:00 -0000 From: Marc Glisse To: GCC Patches Subject: Re: update address taken: don't drop clobbers In-Reply-To: Message-ID: References: User-Agent: Alpine 2.11 (DEB 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-SW-Source: 2014-10/txt/msg01361.txt.bz2 On Sun, 7 Sep 2014, Marc Glisse wrote: > On Sun, 27 Jul 2014, Marc Glisse wrote: > >> On Thu, 10 Jul 2014, Richard Biener wrote: >> >>>> --- gcc/tree-into-ssa.c (revision 212109) >>>> +++ gcc/tree-into-ssa.c (working copy) >>>> @@ -1831,26 +1831,38 @@ maybe_register_def (def_operand_p def_p, >>>> { >>>> tree def = DEF_FROM_PTR (def_p); >>>> tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def); >>>> >>>> /* If DEF is a naked symbol that needs renaming, create a new >>>> name for it. */ >>>> if (marked_for_renaming (sym)) >>>> { >>>> if (DECL_P (def)) >>>> { >>>> - tree tracked_var; >>>> - >>>> - def = make_ssa_name (def, stmt); >>>> + if (gimple_clobber_p (stmt) && is_gimple_reg (sym)) >>> >>> sym should always be a gimple reg here (it's marked for renaming). >>> >>>> + { >>>> + /* Replace clobber stmts with a default def. Create a new >>>> + variable so we don't later think we must coalesce, which >>>> would >>>> + fail with some ada abnormal PHIs. Still, we try to keep >>>> a >>>> + similar name so error messages make sense. */ >>>> + unlink_stmt_vdef (stmt); >>> >>> I think that's redundant with gsi_replace (note that using gsi_replace >>> looks dangerous here as it calls update_stmt during SSA rewrite... >>> that might open a can of worms). >>> >>>> + gsi_replace (&gsi, gimple_build_nop (), true); >>>> + tree id = DECL_NAME (sym); >>>> + const char* name = id ? IDENTIFIER_POINTER (id) : 0; >>>> + tree newvar = create_tmp_var (TREE_TYPE (sym), name); >>>> + def = get_or_create_ssa_default_def (cfun, newvar); >>> >>> So - can't you simply do >>> >>> gimple_assign_set_rhs_from_tree (&gsi, >>> get_or_create_dda_default_def (cfun, sym)); >>> >>> ? Thus replace x = CLOBBER; with x_3 = x_2(D); >>> >>>> + } >>>> + else >>> >>> and of course still rewrite the DEF then. IMHO the copy-propagation >>> you do is premature optimization. >> >> Using your version, I end up with spurious warnings, in particular for >> va_list. pass_fold_builtins stops va_start/va_end taking the address of the >> list, so we get: >> >> list_6 = list_2(D); >> >> in place of the clobber at the end of the function. And there is no >> DCE-like pass afterwards, so we warn for the use of list_2(D). >> (passes.def contains a comment about running dce before uninit) >> >> I don't know if update_address_taken could avoid generating this assignment >> where the lhs has 0 use, but this shows the optimization is not completely >> premature. >> >> (uninit could also check for this case, but that feels like a bad hack) > > I would like some guidance on this. I just tried this trivial patch: > > NEXT_PASS (pass_split_crit_edges); > + NEXT_PASS (pass_dce); > NEXT_PASS (pass_late_warn_uninitialized); > > and it does not cause any regression, it even XPASS gfortran.dg/reassoc_6.f > for some reason. The FIXME note just above in passes.def mentions 2 testcases > that are already xfailed anyway. > > Would that extra pass be acceptable? > > Otherwise, what do you think should be responsible for cleaning up the dead > assignments? Does anyone have an opinion on which side needs to be improved? As a reminder: - we have a va_list with its address taken by va_start/va_end. - fab lowers va_start/va_end and the list doesn't have its address taken anymore. - update_address_taken replaces the clobber: list =v {}; with an assignment of an undefined value: list_6 = list_2(D); - uninit warns about this. Some possible directions: - "prematurely" optimize in update_address_taken so we don't generate the useless assignment. - add a dce pass before uninit. -- Marc Glisse