* [PATCH][20/n] Remove GENERIC stmt combining from SCCVN
@ 2015-07-24 12:45 Richard Biener
2015-09-12 16:35 ` Eric Botcazou
0 siblings, 1 reply; 8+ messages in thread
From: Richard Biener @ 2015-07-24 12:45 UTC (permalink / raw)
To: gcc-patches
This moves simplifying of comparisons against the highest or lowest
possible integer.
Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
This needs the GENERIC code-gen fix, otherwise we miscompile GCC.
Richard.
2015-07-24 Richard Biener <rguenther@suse.de>
* fold-const.c (fold_binary_loc): Move simplifying of comparisons
against the highest or lowest possible integer ...
* match.pd: ... as patterns here.
Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c (revision 226140)
+++ gcc/fold-const.c (working copy)
@@ -11651,123 +11463,6 @@ fold_binary_loc (location_t loc,
}
}
- /* Comparisons with the highest or lowest possible integer of
- the specified precision will have known values. */
- {
- tree arg1_type = TREE_TYPE (arg1);
- unsigned int prec = TYPE_PRECISION (arg1_type);
-
- if (TREE_CODE (arg1) == INTEGER_CST
- && (INTEGRAL_TYPE_P (arg1_type) || POINTER_TYPE_P (arg1_type)))
- {
- wide_int max = wi::max_value (arg1_type);
- wide_int signed_max = wi::max_value (prec, SIGNED);
- wide_int min = wi::min_value (arg1_type);
-
- if (wi::eq_p (arg1, max))
- switch (code)
- {
- case GT_EXPR:
- return omit_one_operand_loc (loc, type, integer_zero_node, arg0);
-
- case GE_EXPR:
- return fold_build2_loc (loc, EQ_EXPR, type, op0, op1);
-
- case LE_EXPR:
- return omit_one_operand_loc (loc, type, integer_one_node, arg0);
-
- case LT_EXPR:
- return fold_build2_loc (loc, NE_EXPR, type, op0, op1);
-
- /* The GE_EXPR and LT_EXPR cases above are not normally
- reached because of previous transformations. */
-
- default:
- break;
- }
- else if (wi::eq_p (arg1, max - 1))
- switch (code)
- {
- case GT_EXPR:
- arg1 = const_binop (PLUS_EXPR, arg1,
- build_int_cst (TREE_TYPE (arg1), 1));
- return fold_build2_loc (loc, EQ_EXPR, type,
- fold_convert_loc (loc,
- TREE_TYPE (arg1), arg0),
- arg1);
- case LE_EXPR:
- arg1 = const_binop (PLUS_EXPR, arg1,
- build_int_cst (TREE_TYPE (arg1), 1));
- return fold_build2_loc (loc, NE_EXPR, type,
- fold_convert_loc (loc, TREE_TYPE (arg1),
- arg0),
- arg1);
- default:
- break;
- }
- else if (wi::eq_p (arg1, min))
- switch (code)
- {
- case LT_EXPR:
- return omit_one_operand_loc (loc, type, integer_zero_node, arg0);
-
- case LE_EXPR:
- return fold_build2_loc (loc, EQ_EXPR, type, op0, op1);
-
- case GE_EXPR:
- return omit_one_operand_loc (loc, type, integer_one_node, arg0);
-
- case GT_EXPR:
- return fold_build2_loc (loc, NE_EXPR, type, op0, op1);
-
- default:
- break;
- }
- else if (wi::eq_p (arg1, min + 1))
- switch (code)
- {
- case GE_EXPR:
- arg1 = const_binop (MINUS_EXPR, arg1,
- build_int_cst (TREE_TYPE (arg1), 1));
- return fold_build2_loc (loc, NE_EXPR, type,
- fold_convert_loc (loc,
- TREE_TYPE (arg1), arg0),
- arg1);
- case LT_EXPR:
- arg1 = const_binop (MINUS_EXPR, arg1,
- build_int_cst (TREE_TYPE (arg1), 1));
- return fold_build2_loc (loc, EQ_EXPR, type,
- fold_convert_loc (loc, TREE_TYPE (arg1),
- arg0),
- arg1);
- default:
- break;
- }
-
- else if (wi::eq_p (arg1, signed_max)
- && TYPE_UNSIGNED (arg1_type)
- /* We will flip the signedness of the comparison operator
- associated with the mode of arg1, so the sign bit is
- specified by this mode. Check that arg1 is the signed
- max associated with this sign bit. */
- && prec == GET_MODE_PRECISION (TYPE_MODE (arg1_type))
- /* signed_type does not work on pointer types. */
- && INTEGRAL_TYPE_P (arg1_type))
- {
- /* The following case also applies to X < signed_max+1
- and X >= signed_max+1 because previous transformations. */
- if (code == LE_EXPR || code == GT_EXPR)
- {
- tree st = signed_type_for (arg1_type);
- return fold_build2_loc (loc,
- code == LE_EXPR ? GE_EXPR : LT_EXPR,
- type, fold_convert_loc (loc, st, arg0),
- build_int_cst (st, 0));
- }
- }
- }
- }
-
/* If we are comparing an ABS_EXPR with a constant, we can
convert all the cases into explicit comparisons, but they may
well not be faster than doing the ABS and one comparison.
Index: gcc/match.pd
===================================================================
--- gcc/match.pd (revision 226140)
+++ gcc/match.pd (working copy)
@@ -1807,6 +1864,73 @@ (define_operator_list CBRT BUILT_IN_CBRT
{ constant_boolean_node (cmp == NE_EXPR, type); })))
+/* Non-equality compare simplifications from fold_binary */
+(for cmp (lt gt le ge)
+ /* Comparisons with the highest or lowest possible integer of
+ the specified precision will have known values. */
+ (simplify
+ (cmp (convert?@2 @0) INTEGER_CST@1)
+ (if ((INTEGRAL_TYPE_P (TREE_TYPE (@1)) || POINTER_TYPE_P (TREE_TYPE (@1)))
+ && tree_nop_conversion_p (TREE_TYPE (@2), TREE_TYPE (@0)))
+ (with
+ {
+ tree arg1_type = TREE_TYPE (@1);
+ unsigned int prec = TYPE_PRECISION (arg1_type);
+ wide_int max = wi::max_value (arg1_type);
+ wide_int signed_max = wi::max_value (prec, SIGNED);
+ wide_int min = wi::min_value (arg1_type);
+ }
+ (switch
+ (if (wi::eq_p (@1, max))
+ (switch
+ (if (cmp == GT_EXPR)
+ { constant_boolean_node (false, type); })
+ (if (cmp == GE_EXPR)
+ (eq @2 @1))
+ (if (cmp == LE_EXPR)
+ { constant_boolean_node (true, type); })
+ (if (cmp == LT_EXPR)
+ (ne @2 @1))))
+ (if (wi::eq_p (@1, max - 1))
+ (switch
+ (if (cmp == GT_EXPR)
+ (eq @2 { wide_int_to_tree (TREE_TYPE (@1), wi::add (@1, 1)); }))
+ (if (cmp == LE_EXPR)
+ (ne @2 { wide_int_to_tree (TREE_TYPE (@1), wi::add (@1, 1)); }))))
+ (if (wi::eq_p (@1, min))
+ (switch
+ (if (cmp == LT_EXPR)
+ { constant_boolean_node (false, type); })
+ (if (cmp == LE_EXPR)
+ (eq @2 @1))
+ (if (cmp == GE_EXPR)
+ { constant_boolean_node (true, type); })
+ (if (cmp == GT_EXPR)
+ (ne @2 @1))))
+ (if (wi::eq_p (@1, min + 1))
+ (switch
+ (if (cmp == GE_EXPR)
+ (ne @2 { wide_int_to_tree (TREE_TYPE (@1), wi::sub (@1, 1)); }))
+ (if (cmp == LT_EXPR)
+ (eq @2 { wide_int_to_tree (TREE_TYPE (@1), wi::sub (@1, 1)); }))))
+ (if (wi::eq_p (@1, signed_max)
+ && TYPE_UNSIGNED (arg1_type)
+ /* We will flip the signedness of the comparison operator
+ associated with the mode of @1, so the sign bit is
+ specified by this mode. Check that @1 is the signed
+ max associated with this sign bit. */
+ && prec == GET_MODE_PRECISION (TYPE_MODE (arg1_type))
+ /* signed_type does not work on pointer types. */
+ && INTEGRAL_TYPE_P (arg1_type))
+ /* The following case also applies to X < signed_max+1
+ and X >= signed_max+1 because previous transformations. */
+ (if (cmp == LE_EXPR || cmp == GT_EXPR)
+ (with { tree st = signed_type_for (arg1_type); }
+ (if (cmp == LE_EXPR)
+ (ge (convert:st @0) { build_zero_cst (st); })
+ (lt (convert:st @0) { build_zero_cst (st); }))))))))))
+
+
/* bool_var != 0 becomes bool_var. */
(simplify
(ne @0 integer_zerop@1)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][20/n] Remove GENERIC stmt combining from SCCVN
2015-07-24 12:45 [PATCH][20/n] Remove GENERIC stmt combining from SCCVN Richard Biener
@ 2015-09-12 16:35 ` Eric Botcazou
2015-09-14 8:52 ` Richard Biener
0 siblings, 1 reply; 8+ messages in thread
From: Eric Botcazou @ 2015-09-12 16:35 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 437 bytes --]
> * fold-const.c (fold_binary_loc): Move simplifying of comparisons
> against the highest or lowest possible integer ...
> * match.pd: ... as patterns here.
This incorrectly dropped the calls to omit_one_operand_loc, resulting in the
failure of the attached Ada test: if the operand has side effects, you cannot
replace the entire comparison with just 'true' or 'false'.
* gnat.dg/overflow_sum3.adb: New test.
--
Eric Botcazou
[-- Attachment #2: overflow_sum3.adb --]
[-- Type: text/x-adasrc, Size: 340 bytes --]
-- { dg-do run }
-- { dg-options "-gnato" }
procedure Overflow_Sum3 is
function Ident (I : Integer) return Integer is
begin
return I;
end;
X : Short_Short_Integer := Short_Short_Integer (Ident (127));
begin
if X+1 <= 127 then
raise Program_Error;
end if;
exception
when Constraint_Error => null;
end;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][20/n] Remove GENERIC stmt combining from SCCVN
2015-09-12 16:35 ` Eric Botcazou
@ 2015-09-14 8:52 ` Richard Biener
2015-09-14 9:00 ` Richard Biener
2015-09-14 9:05 ` Eric Botcazou
0 siblings, 2 replies; 8+ messages in thread
From: Richard Biener @ 2015-09-14 8:52 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
On Sat, 12 Sep 2015, Eric Botcazou wrote:
> > * fold-const.c (fold_binary_loc): Move simplifying of comparisons
> > against the highest or lowest possible integer ...
> > * match.pd: ... as patterns here.
>
> This incorrectly dropped the calls to omit_one_operand_loc, resulting in the
> failure of the attached Ada test: if the operand has side effects, you cannot
> replace the entire comparison with just 'true' or 'false'.
Still trying to reproduce, but I suppose you hit
/* Comparisons with the highest or lowest possible integer of
the specified precision will have known values. */
(simplify
(cmp (convert?@2 @0) INTEGER_CST@1)
(if ((INTEGRAL_TYPE_P (TREE_TYPE (@1)) || POINTER_TYPE_P (TREE_TYPE
(@1)))
&& tree_nop_conversion_p (TREE_TYPE (@2), TREE_TYPE (@0)))
(with
{
tree arg1_type = TREE_TYPE (@1);
unsigned int prec = TYPE_PRECISION (arg1_type);
wide_int max = wi::max_value (arg1_type);
wide_int signed_max = wi::max_value (prec, SIGNED);
wide_int min = wi::min_value (arg1_type);
}
(switch
(if (wi::eq_p (@1, max))
(switch
(if (cmp == GT_EXPR)
{ constant_boolean_node (false, type); })
(if (cmp == GE_EXPR)
(eq @2 @1))
(if (cmp == LE_EXPR)
{ constant_boolean_node (true, type); })
this which should handle side-effects in @0 just fine:
/* #line 2019 "/space/rguenther/src/svn/trunk/gcc/match.pd" */
if (cmp == LE_EXPR)
{
if (dump_file && (dump_flags & TDF_DETAILS))
fprintf (dump_file, "Applying pattern match.pd:2020, %s:%d\n", __FILE__,
__LINE__);
tree res;
res = constant_boolean_node (true, type);
if (TREE_SIDE_EFFECTS (captures[0]))
res = build2_loc (loc, COMPOUND_EXPR, type,
fold_ignored_result (captures[0]), res);
return res;
note that genmatch "inlines" omit_one_operand, so you only see
fold_ignored_result here.
So maybe the issue is with some other pattern or was latent
elsewehere. I'll have a closer look once I manage to reproduce
the issue.
Richard.
>
> * gnat.dg/overflow_sum3.adb: New test.
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][20/n] Remove GENERIC stmt combining from SCCVN
2015-09-14 8:52 ` Richard Biener
@ 2015-09-14 9:00 ` Richard Biener
2015-09-14 9:47 ` Eric Botcazou
2015-09-14 9:05 ` Eric Botcazou
1 sibling, 1 reply; 8+ messages in thread
From: Richard Biener @ 2015-09-14 9:00 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
On Mon, 14 Sep 2015, Richard Biener wrote:
> On Sat, 12 Sep 2015, Eric Botcazou wrote:
>
> > > * fold-const.c (fold_binary_loc): Move simplifying of comparisons
> > > against the highest or lowest possible integer ...
> > > * match.pd: ... as patterns here.
> >
> > This incorrectly dropped the calls to omit_one_operand_loc, resulting in the
> > failure of the attached Ada test: if the operand has side effects, you cannot
> > replace the entire comparison with just 'true' or 'false'.
>
> Still trying to reproduce, but I suppose you hit
>
> /* Comparisons with the highest or lowest possible integer of
> the specified precision will have known values. */
> (simplify
> (cmp (convert?@2 @0) INTEGER_CST@1)
> (if ((INTEGRAL_TYPE_P (TREE_TYPE (@1)) || POINTER_TYPE_P (TREE_TYPE
> (@1)))
> && tree_nop_conversion_p (TREE_TYPE (@2), TREE_TYPE (@0)))
> (with
> {
> tree arg1_type = TREE_TYPE (@1);
> unsigned int prec = TYPE_PRECISION (arg1_type);
> wide_int max = wi::max_value (arg1_type);
> wide_int signed_max = wi::max_value (prec, SIGNED);
> wide_int min = wi::min_value (arg1_type);
> }
> (switch
> (if (wi::eq_p (@1, max))
> (switch
> (if (cmp == GT_EXPR)
> { constant_boolean_node (false, type); })
> (if (cmp == GE_EXPR)
> (eq @2 @1))
> (if (cmp == LE_EXPR)
> { constant_boolean_node (true, type); })
>
> this which should handle side-effects in @0 just fine:
>
> /* #line 2019 "/space/rguenther/src/svn/trunk/gcc/match.pd" */
> if (cmp == LE_EXPR)
> {
> if (dump_file && (dump_flags & TDF_DETAILS))
> fprintf (dump_file, "Applying pattern match.pd:2020, %s:%d\n", __FILE__,
> __LINE__);
> tree res;
> res = constant_boolean_node (true, type);
> if (TREE_SIDE_EFFECTS (captures[0]))
> res = build2_loc (loc, COMPOUND_EXPR, type,
> fold_ignored_result (captures[0]), res);
> return res;
>
> note that genmatch "inlines" omit_one_operand, so you only see
> fold_ignored_result here.
>
> So maybe the issue is with some other pattern or was latent
> elsewehere. I'll have a closer look once I manage to reproduce
> the issue.
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)
<cond_expr 0x7ffff68cbf90
type <integer_type 0x7ffff6572dc8 short_short_integer sizes-gimplified
public visited QI
size <integer_cst 0x7ffff68ccca8 constant visited 8>
unit size <integer_cst 0x7ffff68cccc0 constant visited 1>
align 8 symtab 0 alias set -1 canonical type 0x7ffff6572dc8
precision 8 min <integer_cst 0x7ffff656a678 -128> max <integer_cst
0x7ffff656a6c0 127> context <translation_unit_decl 0x7ffff7ff81e0 D.24> RM
size <integer_cst 0x7ffff68ccca8 8>
chain <type_decl 0x7ffff6900b48 short_short_integer>>
arg 0 <eq_expr 0x7ffff6573938
...
arg 1 <compound_expr 0x7ffff65739b0 type <integer_type 0x7ffff6572dc8
short_short_integer>
side-effects
...
arg 2 <plus_expr 0x7ffff6573910 type <integer_type 0x7ffff6572dc8
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 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=<eq_expr 0x7ffff6573938>,
gnu_expr=<plus_expr 0x7ffff6573910>, 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=<eq_expr 0x7ffff6573938>,
gnu_expr=<plus_expr 0x7ffff6573910>, 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 }
Richard.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][20/n] Remove GENERIC stmt combining from SCCVN
2015-09-14 8:52 ` Richard Biener
2015-09-14 9:00 ` Richard Biener
@ 2015-09-14 9:05 ` Eric Botcazou
1 sibling, 0 replies; 8+ messages in thread
From: Eric Botcazou @ 2015-09-14 9:05 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
> Still trying to reproduce, but I suppose you hit
The testcase fails as of r227729 on x86-64/Linux.
> /* Comparisons with the highest or lowest possible integer of
> the specified precision will have known values. */
> (simplify
> (cmp (convert?@2 @0) INTEGER_CST@1)
> (if ((INTEGRAL_TYPE_P (TREE_TYPE (@1)) || POINTER_TYPE_P (TREE_TYPE
> (@1)))
> && tree_nop_conversion_p (TREE_TYPE (@2), TREE_TYPE (@0)))
> (with
> {
> tree arg1_type = TREE_TYPE (@1);
> unsigned int prec = TYPE_PRECISION (arg1_type);
> wide_int max = wi::max_value (arg1_type);
> wide_int signed_max = wi::max_value (prec, SIGNED);
> wide_int min = wi::min_value (arg1_type);
> }
> (switch
> (if (wi::eq_p (@1, max))
> (switch
> (if (cmp == GT_EXPR)
> { constant_boolean_node (false, type); })
> (if (cmp == GE_EXPR)
> (eq @2 @1))
> (if (cmp == LE_EXPR)
> { constant_boolean_node (true, type); })
>
> this which should handle side-effects in @0 just fine:
>
> /* #line 2019 "/space/rguenther/src/svn/trunk/gcc/match.pd" */
> if (cmp == LE_EXPR)
> {
> if (dump_file && (dump_flags & TDF_DETAILS))
> fprintf (dump_file, "Applying pattern match.pd:2020, %s:%d\n", __FILE__,
> __LINE__);
> tree res;
> res = constant_boolean_node (true, type);
> if (TREE_SIDE_EFFECTS (captures[0]))
> res = build2_loc (loc, COMPOUND_EXPR, type,
> fold_ignored_result (captures[0]), res);
> return res;
>
> note that genmatch "inlines" omit_one_operand, so you only see
> fold_ignored_result here.
I see, then for some reason TREE_SIDE_EFFECTS is not set here.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][20/n] Remove GENERIC stmt combining from SCCVN
2015-09-14 9:00 ` Richard Biener
@ 2015-09-14 9:47 ` Eric Botcazou
2015-09-14 9:57 ` Richard Biener
0 siblings, 1 reply; 8+ messages in thread
From: Eric Botcazou @ 2015-09-14 9:47 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
> 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)
> <cond_expr 0x7ffff68cbf90
> type <integer_type 0x7ffff6572dc8 short_short_integer sizes-gimplified
> public visited QI
> size <integer_cst 0x7ffff68ccca8 constant visited 8>
> unit size <integer_cst 0x7ffff68cccc0 constant visited 1>
> align 8 symtab 0 alias set -1 canonical type 0x7ffff6572dc8
> precision 8 min <integer_cst 0x7ffff656a678 -128> max <integer_cst
> 0x7ffff656a6c0 127> context <translation_unit_decl 0x7ffff7ff81e0 D.24> RM
> size <integer_cst 0x7ffff68ccca8 8>
> chain <type_decl 0x7ffff6900b48 short_short_integer>>
>
> arg 0 <eq_expr 0x7ffff6573938
> ...
> arg 1 <compound_expr 0x7ffff65739b0 type <integer_type 0x7ffff6572dc8
> short_short_integer>
> side-effects
> ...
> arg 2 <plus_expr 0x7ffff6573910 type <integer_type 0x7ffff6572dc8
> 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 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=<eq_expr 0x7ffff6573938>,
> gnu_expr=<plus_expr 0x7ffff6573910>, 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=<eq_expr 0x7ffff6573938>,
> gnu_expr=<plus_expr 0x7ffff6573910>, 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.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][20/n] Remove GENERIC stmt combining from SCCVN
2015-09-14 9:47 ` Eric Botcazou
@ 2015-09-14 9:57 ` Richard Biener
2015-09-15 7:43 ` Eric Botcazou
0 siblings, 1 reply; 8+ messages in thread
From: Richard Biener @ 2015-09-14 9:57 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
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)
> > <cond_expr 0x7ffff68cbf90
> > type <integer_type 0x7ffff6572dc8 short_short_integer sizes-gimplified
> > public visited QI
> > size <integer_cst 0x7ffff68ccca8 constant visited 8>
> > unit size <integer_cst 0x7ffff68cccc0 constant visited 1>
> > align 8 symtab 0 alias set -1 canonical type 0x7ffff6572dc8
> > precision 8 min <integer_cst 0x7ffff656a678 -128> max <integer_cst
> > 0x7ffff656a6c0 127> context <translation_unit_decl 0x7ffff7ff81e0 D.24> RM
> > size <integer_cst 0x7ffff68ccca8 8>
> > chain <type_decl 0x7ffff6900b48 short_short_integer>>
> >
> > arg 0 <eq_expr 0x7ffff6573938
> > ...
> > arg 1 <compound_expr 0x7ffff65739b0 type <integer_type 0x7ffff6572dc8
> > short_short_integer>
> > side-effects
> > ...
> > arg 2 <plus_expr 0x7ffff6573910 type <integer_type 0x7ffff6572dc8
> > 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=<eq_expr 0x7ffff6573938>,
> > gnu_expr=<plus_expr 0x7ffff6573910>, 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=<eq_expr 0x7ffff6573938>,
> > gnu_expr=<plus_expr 0x7ffff6573910>, 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 <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][20/n] Remove GENERIC stmt combining from SCCVN
2015-09-14 9:57 ` Richard Biener
@ 2015-09-15 7:43 ` Eric Botcazou
0 siblings, 0 replies; 8+ messages in thread
From: Eric Botcazou @ 2015-09-15 7:43 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
> > 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).
I think that the testcase is valid (it is derived from a DEC Ada test written
circa 1990). But, yes, the quoted gigi code wants to implement the famous
11.6 clause, whereby a check need not be raised if the expression is dead.
The problem here is that the expression drives a conditional construct with
side effects, so it cannot be considered as dead.
In the end, the code indeed appears to be incorrect in resetting the flag.
Instead the implementation of the 11.6 clause should rely purely on the
optimizers and -fdelete-dead-exceptions.
Thanks for your help to analyze this.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-09-15 7:34 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-24 12:45 [PATCH][20/n] Remove GENERIC stmt combining from SCCVN Richard Biener
2015-09-12 16:35 ` Eric Botcazou
2015-09-14 8:52 ` Richard Biener
2015-09-14 9:00 ` Richard Biener
2015-09-14 9:47 ` Eric Botcazou
2015-09-14 9:57 ` Richard Biener
2015-09-15 7:43 ` Eric Botcazou
2015-09-14 9:05 ` Eric Botcazou
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).