| Message ID | 12981441.W0lox0l1Ju@polaris |
|---|---|
| State | New |
| Headers | show |
On Wed, Apr 19, 2017 at 04:59:50PM +0200, Eric Botcazou wrote: > > I know this attempts to be a copy of what is used elsewhere, but > > at least there it is a result of wi::sub etc. > > Wouldn't it be simpler to > > if (sgn == SIGNED && wi::neg_p (min_op1) && wi::neg_p (wmin)) > > min_ovf = 1; > > else if (sgn == UNSIGNED && wi::ne_p (min_op1, 0)) > > min_ovf = -1; > > > > I mean, for SIGNED if min_op1 is 0, then wmin is 0 to and we want > > min_ovf = 0; > > If it is positive, wmin will be surely negative and again we want > > min_ovf = 0. Only if it is negative and its negation is negative > > too we want min_ovf = 1 (i.e. wi::cmps (0, most_negative) result). > > For UNSIGNED, if min_op1 is 0, again all 3 wi::cmp will yield > > 0 and min_ovf = 0. If it is non-zero, it is > 0, therefore it > > the first wi::cmp will return -1, the second wi::cmp returns > > 1 and the third one -1. > > Fine with me. > > > Is that what we want (e.g. the UNSIGNED case to overflow pretty much always > > except for 0 which should be optimized away anyway)? > > I think so, you'd better be very cautious with overflow and symbolic ranges. > > > Or, shouldn't we just set if (!min_op0 && min_op1 && minus_p) min_op0 = > > build_int_cst (expr_type, 0); before the if (min_op0 && min_op1) case > > and don't duplicate that? > > This isn't better than my version IMO. > > Tested on x86_64-suse-linux, OK for mainline and 6 branch? Ok, thanks. Jakub
On 04/19/2017 08:59 AM, Eric Botcazou wrote: >> I know this attempts to be a copy of what is used elsewhere, but >> at least there it is a result of wi::sub etc. >> Wouldn't it be simpler to >> if (sgn == SIGNED && wi::neg_p (min_op1) && wi::neg_p (wmin)) >> min_ovf = 1; >> else if (sgn == UNSIGNED && wi::ne_p (min_op1, 0)) >> min_ovf = -1; >> >> I mean, for SIGNED if min_op1 is 0, then wmin is 0 to and we want >> min_ovf = 0; >> If it is positive, wmin will be surely negative and again we want >> min_ovf = 0. Only if it is negative and its negation is negative >> too we want min_ovf = 1 (i.e. wi::cmps (0, most_negative) result). >> For UNSIGNED, if min_op1 is 0, again all 3 wi::cmp will yield >> 0 and min_ovf = 0. If it is non-zero, it is > 0, therefore it >> the first wi::cmp will return -1, the second wi::cmp returns >> 1 and the third one -1. > > Fine with me. > >> Is that what we want (e.g. the UNSIGNED case to overflow pretty much always >> except for 0 which should be optimized away anyway)? > > I think so, you'd better be very cautious with overflow and symbolic ranges. > >> Or, shouldn't we just set if (!min_op0 && min_op1 && minus_p) min_op0 = >> build_int_cst (expr_type, 0); before the if (min_op0 && min_op1) case >> and don't duplicate that? > > This isn't better than my version IMO. > > Tested on x86_64-suse-linux, OK for mainline and 6 branch? > > > 2017-04-19 Eric Botcazou <ebotcazou@adacore.com> > Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/80426 > * tree-vrp.c (extract_range_from_binary_expr_1): For an additive > operation on symbolic operands, also compute the overflow for the > invariant part when the operation degenerates into a negation. > Jakub ACKd and I'm going to go ahead and install. I'll pull the testsuite bits from a prior version. Thanks, Jeff
Index: tree-vrp.c =================================================================== --- tree-vrp.c (revision 246960) +++ tree-vrp.c (working copy) @@ -2461,7 +2461,20 @@ extract_range_from_binary_expr_1 (value_ else if (min_op0) wmin = min_op0; else if (min_op1) - wmin = minus_p ? wi::neg (min_op1) : min_op1; + { + if (minus_p) + { + wmin = wi::neg (min_op1); + + /* Check for overflow. */ + if (sgn == SIGNED && wi::neg_p (min_op1) && wi::neg_p (wmin)) + min_ovf = 1; + else if (sgn == UNSIGNED && wi::ne_p (min_op1, 0)) + min_ovf = -1; + } + else + wmin = min_op1; + } else wmin = wi::shwi (0, prec); @@ -2489,7 +2502,20 @@ extract_range_from_binary_expr_1 (value_ else if (max_op0) wmax = max_op0; else if (max_op1) - wmax = minus_p ? wi::neg (max_op1) : max_op1; + { + if (minus_p) + { + wmax = wi::neg (max_op1); + + /* Check for overflow. */ + if (sgn == SIGNED && wi::neg_p (max_op1) && wi::neg_p (wmax)) + max_ovf = 1; + else if (sgn == UNSIGNED && wi::ne_p (max_op1, 0)) + max_ovf = -1; + } + else + wmax = max_op1; + } else wmax = wi::shwi (0, prec);