From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 103335 invoked by alias); 14 Sep 2015 09:50:42 -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 103322 invoked by uid 89); 14 Sep 2015 09:50:41 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_40,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Mon, 14 Sep 2015 09:50:40 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 2338CAAC4; Mon, 14 Sep 2015 09:50:36 +0000 (UTC) Date: Mon, 14 Sep 2015 09:57:00 -0000 From: Richard Biener To: Eric Botcazou cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH][20/n] Remove GENERIC stmt combining from SCCVN In-Reply-To: <2169802.F6q7hcMxNO@polaris> Message-ID: References: <2169802.F6q7hcMxNO@polaris> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-SW-Source: 2015-09/txt/msg00921.txt.bz2 On Mon, 14 Sep 2015, Eric Botcazou wrote: > > Ok, so it's folding > > > > x == 127 ? .gnat_rcheck_CE_Overflow_Check ("overflow_sum3.adb", 14);, 0 : > > (short_short_integer) x + 1 > > > > <= 127 > > > > where op0 (the COND_EXPR) does not have TREE_SIDE_EFFECTS set but > > its operand 1 has: > > > > (gdb) p debug_tree (op0) > > > type > public visited QI > > size > > unit size > > align 8 symtab 0 alias set -1 canonical type 0x7ffff6572dc8 > > precision 8 min max > 0x7ffff656a6c0 127> context RM > > size > > chain > > > > > arg 0 > ... > > arg 1 > short_short_integer> > > side-effects > > ... > > arg 2 > short_short_integer> > > ... > > > > that's unexpected to the code generated by genmatch and I don't see > > how omit_one_operand would handle that either. > > The old code was propagating the comparison inside the arms of COND_EXPR > (fold_binary_op_with_conditional_arg) before applying the transformation: > > if ((short_short_integer) x == 127 ? .gnat_rcheck_CE_Overflow_Check > ("overflow_sum3.adb", 14);, 1 : 1) > > The new code does the reverse, but the old behavior can be easily restored: > > Index: fold-const.c > =================================================================== > --- fold-const.c (revision 227729) > +++ fold-const.c (working copy) > @@ -9025,10 +9025,6 @@ fold_binary_loc (location_t loc, > && tree_swap_operands_p (arg0, arg1, true)) > return fold_build2_loc (loc, swap_tree_comparison (code), type, op1, > op0); > > - tem = generic_simplify (loc, code, type, op0, op1); > - if (tem) > - return tem; > - > /* ARG0 is the first operand of EXPR, and ARG1 is the second operand. > > First check for cases where an arithmetic operation is applied to a > @@ -9114,6 +9110,10 @@ fold_binary_loc (location_t loc, > } > } > > + tem = generic_simplify (loc, code, type, op0, op1); > + if (tem) > + return tem; > + > switch (code) > { > case MEM_REF: > > is sufficient to fix the regression. The newly generated code is better though and I can't see how we should allow fold_binary_op_with_conditional_arg to be required for correctness. Iff then the "fix" would not be the above but to move fold_binary_op_with_conditional_arg to match.pd itself. > > The COND_EXPR is originally built with TREE_SIDE_EFFECTS set but: > > > > Hardware watchpoint 7: *$43 > > > > Old value = 65595 > > New value = 59 > > emit_check (gnu_cond=, > > gnu_expr=, reason=10, gnat_node=2320) > > at /space/rguenther/src/svn/trunk/gcc/ada/gcc-interface/trans.c:8823 > > 8823 return gnu_result; > > $45 = 0 > > > > so the Ada frontend resets the flag (improperly?): > > > > emit_check (gnu_cond=, > > gnu_expr=, reason=10, gnat_node=2320) > > at /space/rguenther/src/svn/trunk/gcc/ada/gcc-interface/trans.c:8823 > > 8823 return gnu_result; > > $45 = 0 > > (gdb) l > > 8818 > > 8819 /* GNU_RESULT has side effects if and only if GNU_EXPR has: > > 8820 we don't need to evaluate it just for the check. */ > > 8821 TREE_SIDE_EFFECTS (gnu_result) = TREE_SIDE_EFFECTS (gnu_expr); > > 8822 > > 8823 return gnu_result; > > 8824 } > > That's old code and the comment makes it quite clear why this is done though. Yeah, but then here "we don't need to evaluate it just for the check" applies - the check is dead code as the outer comparison is always false. I think what the code in the Ada frontend tries to achieve is not actually what it does. Or the testcase is invalid (or rather dependent on optimization performed). Richard. -- Richard Biener SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)