From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32510 invoked by alias); 18 Nov 2015 14:04:07 -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 32418 invoked by uid 89); 18 Nov 2015 14:04:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-yk0-f182.google.com Received: from mail-yk0-f182.google.com (HELO mail-yk0-f182.google.com) (209.85.160.182) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 18 Nov 2015 14:04:04 +0000 Received: by ykdr82 with SMTP id r82so64067811ykd.3 for ; Wed, 18 Nov 2015 06:04:02 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.13.202.197 with SMTP id m188mr1757775ywd.305.1447855442329; Wed, 18 Nov 2015 06:04:02 -0800 (PST) Received: by 10.37.93.11 with HTTP; Wed, 18 Nov 2015 06:04:02 -0800 (PST) In-Reply-To: <56468B17.6020903@linaro.org> References: <55ECFC2A.7050908@linaro.org> <56269E01.5010408@linaro.org> <5628BF86.6070503@linaro.org> <562ECA5D.2040008@linaro.org> <56372A31.3080607@linaro.org> <563F192A.7020004@linaro.org> <56468B17.6020903@linaro.org> Date: Wed, 18 Nov 2015 14:04:00 -0000 Message-ID: Subject: Re: [0/7] Type promotion pass and elimination of zext/sext From: Richard Biener To: Kugan Cc: "gcc-patches@gcc.gnu.org" Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2015-11/txt/msg02213.txt.bz2 On Sat, Nov 14, 2015 at 2:15 AM, Kugan wrote: > > Attached is the latest version of the patch. With the patches > 0001-Add-new-SEXT_EXPR-tree-code.patch, > 0002-Add-type-promotion-pass.patch and > 0003-Optimize-ZEXT_EXPR-with-tree-vrp.patch. > > I did bootstrap on ppc64-linux-gnu, aarch64-linux-gnu and > x64-64-linux-gnu and regression testing on ppc64-linux-gnu, > aarch64-linux-gnu arm64-linux-gnu and x64-64-linux-gnu. I ran into three > issues in ppc64-linux-gnu regression testing. There are some other test > cases which needs adjustment for scanning for some patterns that are not > valid now. > > 1. rtl fwprop was going into infinite loop. Works with the following patch: > diff --git a/gcc/fwprop.c b/gcc/fwprop.c > index 16c7981..9cf4f43 100644 > --- a/gcc/fwprop.c > +++ b/gcc/fwprop.c > @@ -948,6 +948,10 @@ try_fwprop_subst (df_ref use, rtx *loc, rtx > new_rtx, rtx_insn *def_insn, > int old_cost = 0; > bool ok; > > + /* Value to be substituted is the same, nothing to do. */ > + if (rtx_equal_p (*loc, new_rtx)) > + return false; > + > update_df_init (def_insn, insn); > > /* forward_propagate_subreg may be operating on an instruction with Which testcase was this on? > 2. gcc.dg/torture/ftrapv-1.c fails > This is because we are checking for the SImode trapping. With the > promotion of the operation to wider mode, this is i think expected. I > think the testcase needs updating. No, it is not expected. As said earlier you need to refrain from promoting integer operations that trap. You can use ! operation_no_trapping_overflow for this. > 3. gcc.dg/sms-3.c fails > It fails with -fmodulo-sched-allow-regmoves and OK when I remove it. I > am looking into it. > > > I also have the following issues based on the previous review (as posted > in the previous patch). Copying again for the review purpose. > > 1. >> you still call promote_ssa on both DEFs and USEs and promote_ssa looks >> at SSA_NAME_DEF_STMT of the passed arg. Please call promote_ssa just >> on DEFs and fixup_uses on USEs. > > I am doing this to promote SSA that are defined with GIMPLE_NOP. Is > there anyway to iterate over this. I have added gcc_assert to make sure > that promote_ssa is called only once. gcc_assert (!ssa_name_info_map->get_or_insert (def)); with --disable-checking this will be compiled away so you need to do the assert in a separate statement. > 2. >> Instead of this you should, in promote_all_stmts, walk over all uses > doing what >> fixup_uses does and then walk over all defs, doing what promote_ssa does. >> >> + case GIMPLE_NOP: >> + { >> + if (SSA_NAME_VAR (def) == NULL) >> + { >> + /* Promote def by fixing its type for anonymous def. */ >> + TREE_TYPE (def) = promoted_type; >> + } >> + else >> + { >> + /* Create a promoted copy of parameters. */ >> + bb = single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)); >> >> I think the uninitialized vars are somewhat tricky and it would be best >> to create a new uninit anonymous SSA name for them. You can >> have SSA_NAME_VAR != NULL and def _not_ being a parameter >> btw. > > I experimented with get_or_create_default_def. Here we have to have a > SSA_NAME_VAR (def) of promoted type. > > In the attached patch I am doing the following and seems to work. Does > this looks OK? > > + } > + else if (TREE_CODE (SSA_NAME_VAR (def)) != PARM_DECL) > + { > + tree var = copy_node (SSA_NAME_VAR (def)); > + TREE_TYPE (var) = promoted_type; > + TREE_TYPE (def) = promoted_type; > + SET_SSA_NAME_VAR_OR_IDENTIFIER (def, var); > + } I believe this will wreck the SSA default-def map so you should do set_ssa_default_def (cfun, SSA_NAME_VAR (def), NULL_TREE); tree var = create_tmp_reg (promoted_type); TREE_TYPE (def) = promoted_type; SET_SSA_NAME_VAR_OR_IDENTIFIER (def, var); set_ssa_default_def (cfun, var, def); instead. > I prefer to promote def as otherwise iterating over the uses and > promoting can look complicated (have to look at all the different types > of stmts again and do the right thing as It was in the earlier version > of this before we move to this approach) > > 3) >> you can also transparently handle constants for the cases where promoting >> is required. At the moment their handling is interwinded with the def > promotion >> code. That makes the whole thing hard to follow. > > > I have updated the comments with: > > +/* Promote constants in STMT to TYPE. If PROMOTE_COND_EXPR is true, > + promote only the constants in conditions part of the COND_EXPR. > + > + We promote the constants when the associated operands are promoted. > + This usually means that we promote the constants when we promote the > + defining stmnts (as part of promote_ssa). However for COND_EXPR, we > + can promote only when we promote the other operand. Therefore, this > + is done during fixup_use. */ > > > 4) > I am handling gimple_debug separately to avoid any code difference with > and without -g option. I have updated the comments for this. > > 5) > I also noticed that tree-ssa-uninit sometimes gives false positives due > to the assumptions > it makes. Is it OK to move this pass before type promotion? I can do the > testings and post a separate patch with this if this OK. Hmm, no, this needs more explanation (like a testcase). > 6) > I also removed the optimization that prevents some of the redundant > truncation/extensions from type promotion pass, as it dosent do much as > of now. I can send a proper follow up patch. Is that OK? Yeah, that sounds fine. > I also did a simple test with coremark for the latest patch. I compared > the code size for coremark for linux-gcc with -Os. Results are as > reported by the "size" utility. I know this doesn't mean much but can > give some indication. > Base with pass Percentage improvement > ============================================================== > arm 10476 10372 0.9927453226 > aarch64 9545 9521 0.2514405448 > ppc64 12236 12052 1.5037593985 > > > After resolving the above issues, I would like propose that we commit > the pass as not enabled by default (even though the patch as it stands > enabled by default - I am doing it for testing purposes). Hmm, we don't like to have passes that are not enabled by default with any optimization level or for any target. Those tend to bitrot quickly :( Did you do any performance measurements yet? Looking over the pass in detail now (again). Thanks, Richard. > Thanks, > Kugan > >