From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id DB5D43858CDA for ; Mon, 23 Oct 2023 08:21:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DB5D43858CDA Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de ARC-Filter: OpenARC Filter v1.0.0 sourceware.org DB5D43858CDA Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=195.135.220.29 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698049289; cv=none; b=Apdgg/X2aVN5/GZXLRzeFPqtzNaVfG+NzOQttvAb960bbk3yD6GFVGNj8mful0P/nich92yuWNxDSQ7apJQtOSC18KV4K+73oP5TfIpOTQopN2GuZGuqtYzHm3iXNSLdTqAXmzCslSUZuRYQOuUFIahysoAsU4CelOr95VnglWc= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698049289; c=relaxed/simple; bh=e7ptUNoS/O3t9SF1prgwBPKdq3cDmvcJ2HJbXpCk2ok=; h=Date:From:To:Subject:Message-ID:MIME-Version; b=nVbatX+2ZYS0whPuCw7itW3iFQCj9sK0B3YU7ZcD4lL7nLK4/KT8u8f+moEKLxeYNH8yDomZCLkT+I8s7edSb4dea9syePTB4ixwIwAIaKiQ5l15w8ebsTFLn/X3A3WFtHMFPiOXMmLHcItQwL1BTaBBOuDhjnz7tji4Bx5mYyU= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 9BE6F1FD74; Mon, 23 Oct 2023 08:21:26 +0000 (UTC) Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 5B78C2CF38; Mon, 23 Oct 2023 08:21:26 +0000 (UTC) Date: Mon, 23 Oct 2023 08:21:26 +0000 (UTC) From: Richard Biener To: Tamar Christina cc: gcc-patches@gcc.gnu.org, nd@arm.com, jlaw@ventanamicro.com Subject: Re: [PATCH] middle-end: don't keep .MEM guard nodes for PHI nodes who dominate loop [PR111860] In-Reply-To: Message-ID: References: User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Level: Authentication-Results: smtp-out2.suse.de; dkim=none; dmarc=none; spf=softfail (smtp-out2.suse.de: 149.44.160.134 is neither permitted nor denied by domain of rguenther@suse.de) smtp.mailfrom=rguenther@suse.de X-Rspamd-Server: rspamd2 X-Spamd-Result: default: False [-2.51 / 50.00]; ARC_NA(0.00)[]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; TO_DN_SOME(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; MIME_GOOD(-0.10)[text/plain]; DMARC_NA(0.20)[suse.de]; NEURAL_HAM_LONG(-3.00)[-1.000]; R_SPF_SOFTFAIL(0.60)[~all:c]; RWL_MAILSPIKE_GOOD(0.00)[149.44.160.134:from]; VIOLATED_DIRECT_SPF(3.50)[]; MX_GOOD(-0.01)[]; NEURAL_HAM_SHORT(-1.00)[-1.000]; RCVD_NO_TLS_LAST(0.10)[]; FROM_EQ_ENVFROM(0.00)[]; R_DKIM_NA(0.20)[]; MIME_TRACE(0.00)[0:+]; RCVD_COUNT_TWO(0.00)[2]; BAYES_HAM(-3.00)[100.00%] X-Spam-Score: -2.51 X-Rspamd-Queue-Id: 9BE6F1FD74 X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_STATUS,KAM_LOTSOFHASH,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 List-Id: On Fri, 20 Oct 2023, Tamar Christina wrote: > Hi All, > > The previous patch tried to remove PHI nodes that dominated the first loop, > however the correct fix is to only remove .MEM nodes. > > This patch thus makes the condition a bit stricter and only tries to remove > MEM phi nodes. > > I couldn't figure out a way to easily determine if a particular PHI is vUSE > related, so the patch does: > > 1. check if the definition is a vDEF and not defined in main loop. > 2. check if the definition is a PHI and not defined in main loop. > 3. check if the definition is a default definition. > > For no 2 and 3 we may misidentify the PHI, in both cases the value is defined > outside of the loop version block which also makes it ok to remove. > > Bootstrapped Regtested on aarch64-none-linux-gnu, powerpc64le-unknown-linux-gnu, > x86_64-none-linux-gnu and no issues. > > Tested all default testsuites. > > Ok for master? > > Thanks, > Tamar > > gcc/ChangeLog: > > PR tree-optimization/111860 > * tree-vect-loop-manip.cc (slpeel_tree_duplicate_loop_to_edge_cfg): > Drop .MEM nodes only. > > gcc/testsuite/ChangeLog: > > PR tree-optimization/111860 > * gcc.dg/vect/pr111860-2.c: New test. > * gcc.dg/vect/pr111860-3.c: New test. > > --- inline copy of patch -- > diff --git a/gcc/testsuite/gcc.dg/vect/pr111860-2.c b/gcc/testsuite/gcc.dg/vect/pr111860-2.c > new file mode 100644 > index 0000000000000000000000000000000000000000..07f64ffb5318c9d7817d46802d123cc9a2d65ec9 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/vect/pr111860-2.c > @@ -0,0 +1,17 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O -fno-tree-sink -ftree-vectorize" } */ > +int buffer_ctrl_ctx_0, buffer_ctrl_p1, buffer_ctrl_cmd; > + > +int > +buffer_ctrl (long ret, int i) > +{ > + switch (buffer_ctrl_cmd) > + { > + case 1: > + buffer_ctrl_ctx_0 = 0; > + for (; i; i++) > + if (buffer_ctrl_p1) > + ret++; > + } > + return ret; > +} > diff --git a/gcc/testsuite/gcc.dg/vect/pr111860-3.c b/gcc/testsuite/gcc.dg/vect/pr111860-3.c > new file mode 100644 > index 0000000000000000000000000000000000000000..07f64ffb5318c9d7817d46802d123cc9a2d65ec9 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/vect/pr111860-3.c > @@ -0,0 +1,17 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O -fno-tree-sink -ftree-vectorize" } */ > +int buffer_ctrl_ctx_0, buffer_ctrl_p1, buffer_ctrl_cmd; > + > +int > +buffer_ctrl (long ret, int i) > +{ > + switch (buffer_ctrl_cmd) > + { > + case 1: > + buffer_ctrl_ctx_0 = 0; > + for (; i; i++) > + if (buffer_ctrl_p1) > + ret++; > + } > + return ret; > +} > diff --git a/gcc/testsuite/gcc.dg/vect/pr111860.c b/gcc/testsuite/gcc.dg/vect/pr111860.c > new file mode 100644 > index 0000000000000000000000000000000000000000..36f0774601040418bc6b7f27c9425b2bf93b18cb > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/vect/pr111860.c > @@ -0,0 +1,16 @@ > +/* { dg-do compile } */ > + > +int optimize_path_n, optimize_path_d; > +int *optimize_path_d_0; > +extern void path_threeOpt( long); > +void optimize_path() { > + int i; > + long length; > + i = 0; > + for (; i <= optimize_path_n; i++) > + optimize_path_d = 0; > + i = 0; > + for (; i < optimize_path_n; i++) > + length += optimize_path_d_0[i]; > + path_threeOpt(length); > +} > diff --git a/gcc/tree-vect-loop-manip.cc b/gcc/tree-vect-loop-manip.cc > index 1f7779b9834c3aef3c6a993fab916224fab03147..fc55278e63f7a48943fdc32c5e207110cf14507e 100644 > --- a/gcc/tree-vect-loop-manip.cc > +++ b/gcc/tree-vect-loop-manip.cc > @@ -1626,13 +1626,33 @@ slpeel_tree_duplicate_loop_to_edge_cfg (class loop *loop, edge loop_exit, > edge temp_e = redirect_edge_and_branch (exit, new_preheader); > flush_pending_stmts (temp_e); > } > - > /* Record the new SSA names in the cache so that we can skip materializing > them again when we fill in the rest of the LCSSA variables. */ > for (auto phi : new_phis) > { > tree new_arg = gimple_phi_arg (phi, 0)->def; > new_phi_args.put (new_arg, gimple_phi_result (phi)); don't you want to skip this as well? > + > + if (!SSA_VAR_P (new_arg)) > + continue; > + /* If the PHI MEM node dominates the loop then we shouldn't create > + a new LC-SSSA PHI for it in the intermediate block. */ > + gimple *def_stmt = SSA_NAME_DEF_STMT (new_arg); > + basic_block def_bb = gimple_bb (def_stmt); > + /* A MEM phi that consitutes a new DEF for the vUSE chain can either > + be a .VDEF or a PHI that operates on MEM. */ > + if (((gimple_vdef (def_stmt) || is_a (def_stmt)) > + /* And said definition must not be inside the main loop. */ > + && (!def_bb || !flow_bb_inside_loop_p (loop, def_bb))) > + /* Or we must be a parameter. In the last two cases we may remove > + a non-MEM PHI node, but since they dominate both loops the > + removal is unlikely to cause trouble as the exits must already > + be using them. */ > + || SSA_NAME_IS_DEFAULT_DEF (new_arg)) > + { > + auto gsi = gsi_for_stmt (phi); > + remove_phi_node (&gsi, true); > + } May I suggest the simpler gimple *def; if (virtual_operand_p (new_arg) && (SSA_NAME_IS_DEFAULT_DEF (new_arg) || !flow_bb_inside_loop_p (loop, gimple_bb (SSA_NAME_DEF_STMT (new_arg)))) { > + auto gsi = gsi_for_stmt (phi); > + remove_phi_node (&gsi, true); } ? OK with that change. Richard.