Re: [PATCH] Fix PR81503 (SLSR invalid fold)
On Aug 28, 2017, at 1:40 PM, Bill Schmidtwrote: > >> >> On Aug 28, 2017, at 12:57 PM, Bill Schmidt >> wrote: >> >> On Aug 28, 2017, at 7:37 AM, Richard Biener >> wrote: >>> >>> On Thu, Aug 3, 2017 at 9:34 PM, Bill Schmidt >>> wrote: Hi, Here's v2 of the patch with Jakub's suggestions incorporated. Bootstrapped and tested on powerpc64le-linux-gnu with no regressions. Is this ok for trunk? Eventually this should be backported to all active releases as well. Ok for that after a week or so of burn-in? (And after 7.2, I imagine.) Thanks, Bill [gcc] 2017-08-03 Bill Schmidt Jakub Jelinek PR tree-optimization/81503 * gimple-ssa-strength-reduction.c (replace_mult_candidate): Ensure folded constant fits in the target type. [gcc/testsuite] 2017-08-03 Bill Schmidt Jakub Jelinek PR tree-optimization/81503 * gcc.c-torture/execute/pr81503.c: New file. Index: gcc/gimple-ssa-strength-reduction.c === --- gcc/gimple-ssa-strength-reduction.c (revision 250791) +++ gcc/gimple-ssa-strength-reduction.c (working copy) @@ -2074,6 +2074,10 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ { tree target_type = TREE_TYPE (gimple_assign_lhs (c->cand_stmt)); enum tree_code cand_code = gimple_assign_rhs_code (c->cand_stmt); + unsigned int prec = TYPE_PRECISION (target_type); + tree maxval = (POINTER_TYPE_P (target_type) +? TYPE_MAX_VALUE (sizetype) +: TYPE_MAX_VALUE (target_type)); /* It is highly unlikely, but possible, that the resulting bump doesn't fit in a HWI. Abandon the replacement @@ -2082,6 +2086,17 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ types but allows for safe negation without twisted logic. */ if (wi::fits_shwi_p (bump) && bump.to_shwi () != HOST_WIDE_INT_MIN + /* It is more likely that the bump doesn't fit in the target +type, so check whether constraining it to that type changes +the value. For a signed type, the value mustn't change. +For an unsigned type, the value may only change to a +congruent value (for negative bumps). */ + && (TYPE_UNSIGNED (target_type) + ? wi::eq_p (wi::neg_p (bump) + ? bump + wi::to_widest (maxval) + 1 + : bump, + wi::zext (bump, prec)) + : wi::eq_p (bump, wi::sext (bump, prec))) >>> >>> Not sure, but would it be fixed in a similar way when writing >>> >>> @@ -2089,16 +2089,9 @@ replace_mult_candidate (slsr_cand_t c, t >>> tree target_type = TREE_TYPE (gimple_assign_lhs (c->cand_stmt)); >>> enum tree_code cand_code = gimple_assign_rhs_code (c->cand_stmt); >>> >>> - /* It is highly unlikely, but possible, that the resulting >>> - bump doesn't fit in a HWI. Abandon the replacement >>> - in this case. This does not affect siblings or dependents >>> - of C. Restriction to signed HWI is conservative for unsigned >>> - types but allows for safe negation without twisted logic. */ >>> - if (wi::fits_shwi_p (bump) >>> - && bump.to_shwi () != HOST_WIDE_INT_MIN >>> - /* It is not useful to replace casts, copies, negates, or adds of >>> -an SSA name and a constant. */ >>> - && cand_code != SSA_NAME >>> + /* It is not useful to replace casts, copies, negates, or adds of >>> + an SSA name and a constant. */ >>> + if (cand_code != SSA_NAME >>> && !CONVERT_EXPR_CODE_P (cand_code) >>> && cand_code != PLUS_EXPR >>> && cand_code != POINTER_PLUS_EXPR >>> @@ -2109,18 +2102,25 @@ replace_mult_candidate (slsr_cand_t c, t >>> tree bump_tree; >>> gimple *stmt_to_print = NULL; >>> >>> - /* If the basis name and the candidate's LHS have incompatible >>> -types, introduce a cast. */ >>> - if (!useless_type_conversion_p (target_type, TREE_TYPE (basis_name))) >>> - basis_name = introduce_cast_before_cand (c, target_type, >>> basis_name); >>> if (wi::neg_p (bump)) >>> { >>>code = MINUS_EXPR; >>>bump = -bump; >>> } >>> + /* It is possible that the resulting bump doesn't fit in target_type. >>> +Abandon the replacement in this case. This does not affect >>> +siblings or dependents of C. */ >>> + if (bump != wi::ext (bump, TYPE_PRECISION (target_type), >>> + TYPE_SIGN (target_type))) >>> + return; >>> >>>
Re: [PATCH] Fix PR81503 (SLSR invalid fold)
> On Aug 28, 2017, at 12:57 PM, Bill Schmidt> wrote: > > On Aug 28, 2017, at 7:37 AM, Richard Biener > wrote: >> >> On Thu, Aug 3, 2017 at 9:34 PM, Bill Schmidt >> wrote: >>> Hi, >>> >>> Here's v2 of the patch with Jakub's suggestions incorporated. Bootstrapped >>> and tested on powerpc64le-linux-gnu with no regressions. Is this ok for >>> trunk? >>> >>> Eventually this should be backported to all active releases as well. >>> Ok for that after a week or so of burn-in? (And after 7.2, I imagine.) >>> >>> Thanks, >>> Bill >>> >>> >>> [gcc] >>> >>> 2017-08-03 Bill Schmidt >>> Jakub Jelinek >>> >>> PR tree-optimization/81503 >>> * gimple-ssa-strength-reduction.c (replace_mult_candidate): Ensure >>> folded constant fits in the target type. >>> >>> [gcc/testsuite] >>> >>> 2017-08-03 Bill Schmidt >>> Jakub Jelinek >>> >>> PR tree-optimization/81503 >>> * gcc.c-torture/execute/pr81503.c: New file. >>> >>> >>> Index: gcc/gimple-ssa-strength-reduction.c >>> === >>> --- gcc/gimple-ssa-strength-reduction.c (revision 250791) >>> +++ gcc/gimple-ssa-strength-reduction.c (working copy) >>> @@ -2074,6 +2074,10 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ >>> { >>> tree target_type = TREE_TYPE (gimple_assign_lhs (c->cand_stmt)); >>> enum tree_code cand_code = gimple_assign_rhs_code (c->cand_stmt); >>> + unsigned int prec = TYPE_PRECISION (target_type); >>> + tree maxval = (POINTER_TYPE_P (target_type) >>> +? TYPE_MAX_VALUE (sizetype) >>> +: TYPE_MAX_VALUE (target_type)); >>> >>> /* It is highly unlikely, but possible, that the resulting >>> bump doesn't fit in a HWI. Abandon the replacement >>> @@ -2082,6 +2086,17 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ >>> types but allows for safe negation without twisted logic. */ >>> if (wi::fits_shwi_p (bump) >>> && bump.to_shwi () != HOST_WIDE_INT_MIN >>> + /* It is more likely that the bump doesn't fit in the target >>> +type, so check whether constraining it to that type changes >>> +the value. For a signed type, the value mustn't change. >>> +For an unsigned type, the value may only change to a >>> +congruent value (for negative bumps). */ >>> + && (TYPE_UNSIGNED (target_type) >>> + ? wi::eq_p (wi::neg_p (bump) >>> + ? bump + wi::to_widest (maxval) + 1 >>> + : bump, >>> + wi::zext (bump, prec)) >>> + : wi::eq_p (bump, wi::sext (bump, prec))) >> >> Not sure, but would it be fixed in a similar way when writing >> >> @@ -2089,16 +2089,9 @@ replace_mult_candidate (slsr_cand_t c, t >> tree target_type = TREE_TYPE (gimple_assign_lhs (c->cand_stmt)); >> enum tree_code cand_code = gimple_assign_rhs_code (c->cand_stmt); >> >> - /* It is highly unlikely, but possible, that the resulting >> - bump doesn't fit in a HWI. Abandon the replacement >> - in this case. This does not affect siblings or dependents >> - of C. Restriction to signed HWI is conservative for unsigned >> - types but allows for safe negation without twisted logic. */ >> - if (wi::fits_shwi_p (bump) >> - && bump.to_shwi () != HOST_WIDE_INT_MIN >> - /* It is not useful to replace casts, copies, negates, or adds of >> -an SSA name and a constant. */ >> - && cand_code != SSA_NAME >> + /* It is not useful to replace casts, copies, negates, or adds of >> + an SSA name and a constant. */ >> + if (cand_code != SSA_NAME >> && !CONVERT_EXPR_CODE_P (cand_code) >> && cand_code != PLUS_EXPR >> && cand_code != POINTER_PLUS_EXPR >> @@ -2109,18 +2102,25 @@ replace_mult_candidate (slsr_cand_t c, t >> tree bump_tree; >> gimple *stmt_to_print = NULL; >> >> - /* If the basis name and the candidate's LHS have incompatible >> -types, introduce a cast. */ >> - if (!useless_type_conversion_p (target_type, TREE_TYPE (basis_name))) >> - basis_name = introduce_cast_before_cand (c, target_type, basis_name); >> if (wi::neg_p (bump)) >> { >> code = MINUS_EXPR; >> bump = -bump; >> } >> + /* It is possible that the resulting bump doesn't fit in target_type. >> +Abandon the replacement in this case. This does not affect >> +siblings or dependents of C. */ >> + if (bump != wi::ext (bump, TYPE_PRECISION (target_type), >> + TYPE_SIGN (target_type))) >> + return; >> >> bump_tree = wide_int_to_tree (target_type, bump); >> >> + /* If the basis name and the candidate's LHS have incompatible >> +types, introduce a cast. */ >> +
Re: [PATCH] Fix PR81503 (SLSR invalid fold)
On Aug 28, 2017, at 7:37 AM, Richard Bienerwrote: > > On Thu, Aug 3, 2017 at 9:34 PM, Bill Schmidt > wrote: >> Hi, >> >> Here's v2 of the patch with Jakub's suggestions incorporated. Bootstrapped >> and tested on powerpc64le-linux-gnu with no regressions. Is this ok for >> trunk? >> >> Eventually this should be backported to all active releases as well. >> Ok for that after a week or so of burn-in? (And after 7.2, I imagine.) >> >> Thanks, >> Bill >> >> >> [gcc] >> >> 2017-08-03 Bill Schmidt >>Jakub Jelinek >> >>PR tree-optimization/81503 >>* gimple-ssa-strength-reduction.c (replace_mult_candidate): Ensure >>folded constant fits in the target type. >> >> [gcc/testsuite] >> >> 2017-08-03 Bill Schmidt >>Jakub Jelinek >> >>PR tree-optimization/81503 >>* gcc.c-torture/execute/pr81503.c: New file. >> >> >> Index: gcc/gimple-ssa-strength-reduction.c >> === >> --- gcc/gimple-ssa-strength-reduction.c (revision 250791) >> +++ gcc/gimple-ssa-strength-reduction.c (working copy) >> @@ -2074,6 +2074,10 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ >> { >> tree target_type = TREE_TYPE (gimple_assign_lhs (c->cand_stmt)); >> enum tree_code cand_code = gimple_assign_rhs_code (c->cand_stmt); >> + unsigned int prec = TYPE_PRECISION (target_type); >> + tree maxval = (POINTER_TYPE_P (target_type) >> +? TYPE_MAX_VALUE (sizetype) >> +: TYPE_MAX_VALUE (target_type)); >> >> /* It is highly unlikely, but possible, that the resulting >> bump doesn't fit in a HWI. Abandon the replacement >> @@ -2082,6 +2086,17 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ >> types but allows for safe negation without twisted logic. */ >> if (wi::fits_shwi_p (bump) >> && bump.to_shwi () != HOST_WIDE_INT_MIN >> + /* It is more likely that the bump doesn't fit in the target >> +type, so check whether constraining it to that type changes >> +the value. For a signed type, the value mustn't change. >> +For an unsigned type, the value may only change to a >> +congruent value (for negative bumps). */ >> + && (TYPE_UNSIGNED (target_type) >> + ? wi::eq_p (wi::neg_p (bump) >> + ? bump + wi::to_widest (maxval) + 1 >> + : bump, >> + wi::zext (bump, prec)) >> + : wi::eq_p (bump, wi::sext (bump, prec))) > > Not sure, but would it be fixed in a similar way when writing > > @@ -2089,16 +2089,9 @@ replace_mult_candidate (slsr_cand_t c, t > tree target_type = TREE_TYPE (gimple_assign_lhs (c->cand_stmt)); > enum tree_code cand_code = gimple_assign_rhs_code (c->cand_stmt); > > - /* It is highly unlikely, but possible, that the resulting > - bump doesn't fit in a HWI. Abandon the replacement > - in this case. This does not affect siblings or dependents > - of C. Restriction to signed HWI is conservative for unsigned > - types but allows for safe negation without twisted logic. */ > - if (wi::fits_shwi_p (bump) > - && bump.to_shwi () != HOST_WIDE_INT_MIN > - /* It is not useful to replace casts, copies, negates, or adds of > -an SSA name and a constant. */ > - && cand_code != SSA_NAME > + /* It is not useful to replace casts, copies, negates, or adds of > + an SSA name and a constant. */ > + if (cand_code != SSA_NAME > && !CONVERT_EXPR_CODE_P (cand_code) > && cand_code != PLUS_EXPR > && cand_code != POINTER_PLUS_EXPR > @@ -2109,18 +2102,25 @@ replace_mult_candidate (slsr_cand_t c, t > tree bump_tree; > gimple *stmt_to_print = NULL; > > - /* If the basis name and the candidate's LHS have incompatible > -types, introduce a cast. */ > - if (!useless_type_conversion_p (target_type, TREE_TYPE (basis_name))) > - basis_name = introduce_cast_before_cand (c, target_type, basis_name); > if (wi::neg_p (bump)) >{ > code = MINUS_EXPR; > bump = -bump; >} > + /* It is possible that the resulting bump doesn't fit in target_type. > +Abandon the replacement in this case. This does not affect > +siblings or dependents of C. */ > + if (bump != wi::ext (bump, TYPE_PRECISION (target_type), > + TYPE_SIGN (target_type))) > + return; > > bump_tree = wide_int_to_tree (target_type, bump); > > + /* If the basis name and the candidate's LHS have incompatible > +types, introduce a cast. */ > + if (!useless_type_conversion_p (target_type, TREE_TYPE (basis_name))) > + basis_name = introduce_cast_before_cand (c, target_type, basis_name); > + > if (dump_file &&
Re: [PATCH] Fix PR81503 (SLSR invalid fold)
On Thu, Aug 3, 2017 at 9:34 PM, Bill Schmidtwrote: > Hi, > > Here's v2 of the patch with Jakub's suggestions incorporated. Bootstrapped > and tested on powerpc64le-linux-gnu with no regressions. Is this ok for > trunk? > > Eventually this should be backported to all active releases as well. > Ok for that after a week or so of burn-in? (And after 7.2, I imagine.) > > Thanks, > Bill > > > [gcc] > > 2017-08-03 Bill Schmidt > Jakub Jelinek > > PR tree-optimization/81503 > * gimple-ssa-strength-reduction.c (replace_mult_candidate): Ensure > folded constant fits in the target type. > > [gcc/testsuite] > > 2017-08-03 Bill Schmidt > Jakub Jelinek > > PR tree-optimization/81503 > * gcc.c-torture/execute/pr81503.c: New file. > > > Index: gcc/gimple-ssa-strength-reduction.c > === > --- gcc/gimple-ssa-strength-reduction.c (revision 250791) > +++ gcc/gimple-ssa-strength-reduction.c (working copy) > @@ -2074,6 +2074,10 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ > { >tree target_type = TREE_TYPE (gimple_assign_lhs (c->cand_stmt)); >enum tree_code cand_code = gimple_assign_rhs_code (c->cand_stmt); > + unsigned int prec = TYPE_PRECISION (target_type); > + tree maxval = (POINTER_TYPE_P (target_type) > +? TYPE_MAX_VALUE (sizetype) > +: TYPE_MAX_VALUE (target_type)); > >/* It is highly unlikely, but possible, that the resulting > bump doesn't fit in a HWI. Abandon the replacement > @@ -2082,6 +2086,17 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ > types but allows for safe negation without twisted logic. */ >if (wi::fits_shwi_p (bump) >&& bump.to_shwi () != HOST_WIDE_INT_MIN > + /* It is more likely that the bump doesn't fit in the target > +type, so check whether constraining it to that type changes > +the value. For a signed type, the value mustn't change. > +For an unsigned type, the value may only change to a > +congruent value (for negative bumps). */ > + && (TYPE_UNSIGNED (target_type) > + ? wi::eq_p (wi::neg_p (bump) > + ? bump + wi::to_widest (maxval) + 1 > + : bump, > + wi::zext (bump, prec)) > + : wi::eq_p (bump, wi::sext (bump, prec))) Not sure, but would it be fixed in a similar way when writing @@ -2089,16 +2089,9 @@ replace_mult_candidate (slsr_cand_t c, t tree target_type = TREE_TYPE (gimple_assign_lhs (c->cand_stmt)); enum tree_code cand_code = gimple_assign_rhs_code (c->cand_stmt); - /* It is highly unlikely, but possible, that the resulting - bump doesn't fit in a HWI. Abandon the replacement - in this case. This does not affect siblings or dependents - of C. Restriction to signed HWI is conservative for unsigned - types but allows for safe negation without twisted logic. */ - if (wi::fits_shwi_p (bump) - && bump.to_shwi () != HOST_WIDE_INT_MIN - /* It is not useful to replace casts, copies, negates, or adds of -an SSA name and a constant. */ - && cand_code != SSA_NAME + /* It is not useful to replace casts, copies, negates, or adds of + an SSA name and a constant. */ + if (cand_code != SSA_NAME && !CONVERT_EXPR_CODE_P (cand_code) && cand_code != PLUS_EXPR && cand_code != POINTER_PLUS_EXPR @@ -2109,18 +2102,25 @@ replace_mult_candidate (slsr_cand_t c, t tree bump_tree; gimple *stmt_to_print = NULL; - /* If the basis name and the candidate's LHS have incompatible -types, introduce a cast. */ - if (!useless_type_conversion_p (target_type, TREE_TYPE (basis_name))) - basis_name = introduce_cast_before_cand (c, target_type, basis_name); if (wi::neg_p (bump)) { code = MINUS_EXPR; bump = -bump; } + /* It is possible that the resulting bump doesn't fit in target_type. +Abandon the replacement in this case. This does not affect +siblings or dependents of C. */ + if (bump != wi::ext (bump, TYPE_PRECISION (target_type), + TYPE_SIGN (target_type))) + return; bump_tree = wide_int_to_tree (target_type, bump); + /* If the basis name and the candidate's LHS have incompatible +types, introduce a cast. */ + if (!useless_type_conversion_p (target_type, TREE_TYPE (basis_name))) + basis_name = introduce_cast_before_cand (c, target_type, basis_name); + if (dump_file && (dump_flags & TDF_DETAILS)) { fputs ("Replacing: ", dump_file); ? >/* It is not useful to replace casts, copies, negates, or adds of > an SSA name and a constant. */ >&& cand_code !=
[PING][PATCH] Fix PR81503 (SLSR invalid fold)
Hi, I'd like to ping this patch, please. Thanks! Bill > On Aug 3, 2017, at 2:34 PM, Bill Schmidtwrote: > > Hi, > > Here's v2 of the patch with Jakub's suggestions incorporated. Bootstrapped > and tested on powerpc64le-linux-gnu with no regressions. Is this ok for > trunk? > > Eventually this should be backported to all active releases as well. > Ok for that after a week or so of burn-in? (And after 7.2, I imagine.) > > Thanks, > Bill > > > [gcc] > > 2017-08-03 Bill Schmidt > Jakub Jelinek > > PR tree-optimization/81503 > * gimple-ssa-strength-reduction.c (replace_mult_candidate): Ensure > folded constant fits in the target type. > > [gcc/testsuite] > > 2017-08-03 Bill Schmidt > Jakub Jelinek > > PR tree-optimization/81503 > * gcc.c-torture/execute/pr81503.c: New file. > > > Index: gcc/gimple-ssa-strength-reduction.c > === > --- gcc/gimple-ssa-strength-reduction.c (revision 250791) > +++ gcc/gimple-ssa-strength-reduction.c (working copy) > @@ -2074,6 +2074,10 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ > { > tree target_type = TREE_TYPE (gimple_assign_lhs (c->cand_stmt)); > enum tree_code cand_code = gimple_assign_rhs_code (c->cand_stmt); > + unsigned int prec = TYPE_PRECISION (target_type); > + tree maxval = (POINTER_TYPE_P (target_type) > + ? TYPE_MAX_VALUE (sizetype) > + : TYPE_MAX_VALUE (target_type)); > > /* It is highly unlikely, but possible, that the resulting > bump doesn't fit in a HWI. Abandon the replacement > @@ -2082,6 +2086,17 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ > types but allows for safe negation without twisted logic. */ > if (wi::fits_shwi_p (bump) > && bump.to_shwi () != HOST_WIDE_INT_MIN > + /* It is more likely that the bump doesn't fit in the target > + type, so check whether constraining it to that type changes > + the value. For a signed type, the value mustn't change. > + For an unsigned type, the value may only change to a > + congruent value (for negative bumps). */ > + && (TYPE_UNSIGNED (target_type) > + ? wi::eq_p (wi::neg_p (bump) > + ? bump + wi::to_widest (maxval) + 1 > + : bump, > + wi::zext (bump, prec)) > + : wi::eq_p (bump, wi::sext (bump, prec))) > /* It is not useful to replace casts, copies, negates, or adds of >an SSA name and a constant. */ > && cand_code != SSA_NAME > Index: gcc/testsuite/gcc.c-torture/execute/pr81503.c > === > --- gcc/testsuite/gcc.c-torture/execute/pr81503.c (nonexistent) > +++ gcc/testsuite/gcc.c-torture/execute/pr81503.c (working copy) > @@ -0,0 +1,15 @@ > +unsigned short a = 41461; > +unsigned short b = 3419; > +int c = 0; > + > +void foo() { > + if (a + b * ~(0 != 5)) > +c = -~(b * ~(0 != 5)) + 2147483647; > +} > + > +int main() { > + foo(); > + if (c != 2147476810) > +return -1; > + return 0; > +} > > > On 8/3/17 1:02 PM, Bill Schmidt wrote: >>> On Aug 3, 2017, at 11:39 AM, Jakub Jelinek wrote: >>> >>> On Thu, Aug 03, 2017 at 11:29:44AM -0500, Bill Schmidt wrote: > And, wouldn't it be more readable to use: >&& (TYPE_UNSIGNED (target_type) > ? (wi::eq_p (bump, wi::zext (bump, prec)) >|| wi::eq_p (bump + wi::to_widest (maxval) + 1, > wi::zext (bump, prec))) > : wi::eq_p (bump, wi::sext (bump, prec))) > ? Probably. As noted, it's all becoming a bit unreadable with too much negative logic in a long conditional, so I want to clean that up in a follow-up. > For TYPE_UNSIGNED, do you actually need any restriction? > What kind of bump values are wrong for unsigned types and why? If the value of the bump is actually larger than the precision of the type (not likely except for quite small types), say 2 * (maxval + 1) which is congruent to 0, the replacement is wrong. >>> Ah, ok. Anyway, for unsigned type, perhaps it could be written as: >>> && (TYPE_UNSIGNED (target_type) >>> ? wi::eq_p (wi::neg_p (bump) ? bump + wi::to_widest (maxval) + 1 >>>: bump, wi::zext (bump, prec)) >>> : wi::eq_p (bump, wi::sext (bump, prec))) >>> I mean, if bump >= 0, then the bump + wi::to_widest (maxval) + 1 >>> value has no chance to be equal to zero extended bump, and >>> for bump < 0 only that one has a chance. >> Yeah, that's true. And arguably my case for the really large bump >> causing problems is kind of thin, because the program is probably >> already broken in that case anyway. But I think I will sleep better >> having
Re: [PATCH] Fix PR81503 (SLSR invalid fold)
Hi, Here's v2 of the patch with Jakub's suggestions incorporated. Bootstrapped and tested on powerpc64le-linux-gnu with no regressions. Is this ok for trunk? Eventually this should be backported to all active releases as well. Ok for that after a week or so of burn-in? (And after 7.2, I imagine.) Thanks, Bill [gcc] 2017-08-03 Bill SchmidtJakub Jelinek PR tree-optimization/81503 * gimple-ssa-strength-reduction.c (replace_mult_candidate): Ensure folded constant fits in the target type. [gcc/testsuite] 2017-08-03 Bill Schmidt Jakub Jelinek PR tree-optimization/81503 * gcc.c-torture/execute/pr81503.c: New file. Index: gcc/gimple-ssa-strength-reduction.c === --- gcc/gimple-ssa-strength-reduction.c (revision 250791) +++ gcc/gimple-ssa-strength-reduction.c (working copy) @@ -2074,6 +2074,10 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ { tree target_type = TREE_TYPE (gimple_assign_lhs (c->cand_stmt)); enum tree_code cand_code = gimple_assign_rhs_code (c->cand_stmt); + unsigned int prec = TYPE_PRECISION (target_type); + tree maxval = (POINTER_TYPE_P (target_type) +? TYPE_MAX_VALUE (sizetype) +: TYPE_MAX_VALUE (target_type)); /* It is highly unlikely, but possible, that the resulting bump doesn't fit in a HWI. Abandon the replacement @@ -2082,6 +2086,17 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ types but allows for safe negation without twisted logic. */ if (wi::fits_shwi_p (bump) && bump.to_shwi () != HOST_WIDE_INT_MIN + /* It is more likely that the bump doesn't fit in the target +type, so check whether constraining it to that type changes +the value. For a signed type, the value mustn't change. +For an unsigned type, the value may only change to a +congruent value (for negative bumps). */ + && (TYPE_UNSIGNED (target_type) + ? wi::eq_p (wi::neg_p (bump) + ? bump + wi::to_widest (maxval) + 1 + : bump, + wi::zext (bump, prec)) + : wi::eq_p (bump, wi::sext (bump, prec))) /* It is not useful to replace casts, copies, negates, or adds of an SSA name and a constant. */ && cand_code != SSA_NAME Index: gcc/testsuite/gcc.c-torture/execute/pr81503.c === --- gcc/testsuite/gcc.c-torture/execute/pr81503.c (nonexistent) +++ gcc/testsuite/gcc.c-torture/execute/pr81503.c (working copy) @@ -0,0 +1,15 @@ +unsigned short a = 41461; +unsigned short b = 3419; +int c = 0; + +void foo() { + if (a + b * ~(0 != 5)) +c = -~(b * ~(0 != 5)) + 2147483647; +} + +int main() { + foo(); + if (c != 2147476810) +return -1; + return 0; +} On 8/3/17 1:02 PM, Bill Schmidt wrote: >> On Aug 3, 2017, at 11:39 AM, Jakub Jelinek wrote: >> >> On Thu, Aug 03, 2017 at 11:29:44AM -0500, Bill Schmidt wrote: And, wouldn't it be more readable to use: && (TYPE_UNSIGNED (target_type) ? (wi::eq_p (bump, wi::zext (bump, prec)) || wi::eq_p (bump + wi::to_widest (maxval) + 1, wi::zext (bump, prec))) : wi::eq_p (bump, wi::sext (bump, prec))) ? >>> Probably. As noted, it's all becoming a bit unreadable with too >>> much negative logic in a long conditional, so I want to clean that >>> up in a follow-up. >>> For TYPE_UNSIGNED, do you actually need any restriction? What kind of bump values are wrong for unsigned types and why? >>> If the value of the bump is actually larger than the precision of the >>> type (not likely except for quite small types), say 2 * (maxval + 1) >>> which is congruent to 0, the replacement is wrong. >> Ah, ok. Anyway, for unsigned type, perhaps it could be written as: >> && (TYPE_UNSIGNED (target_type) >>? wi::eq_p (wi::neg_p (bump) ? bump + wi::to_widest (maxval) + 1 >> : bump, wi::zext (bump, prec)) >>: wi::eq_p (bump, wi::sext (bump, prec))) >> I mean, if bump >= 0, then the bump + wi::to_widest (maxval) + 1 >> value has no chance to be equal to zero extended bump, and >> for bump < 0 only that one has a chance. > Yeah, that's true. And arguably my case for the really large bump > causing problems is kind of thin, because the program is probably > already broken in that case anyway. But I think I will sleep better > having the check in there, as somebody other than SLSR will catch > the bug then. ;-) > > Thanks for all the help with this one. These corner cases are > always tricky, and I appreciate the extra eyeballs. > > Bill > >> Jakub >>
Re: [PATCH] Fix PR81503 (SLSR invalid fold)
-- Bill Bill Schmidt, Ph.D. GCC for Linux on Power Linux on Power Toolchain IBM Linux Technology Center wschm...@linux.vnet.ibm.com > On Aug 3, 2017, at 11:39 AM, Jakub Jelinekwrote: > > On Thu, Aug 03, 2017 at 11:29:44AM -0500, Bill Schmidt wrote: >>> And, wouldn't it be more readable to use: >>> && (TYPE_UNSIGNED (target_type) >>> ? (wi::eq_p (bump, wi::zext (bump, prec)) >>> || wi::eq_p (bump + wi::to_widest (maxval) + 1, >>> wi::zext (bump, prec))) >>> : wi::eq_p (bump, wi::sext (bump, prec))) >>> ? >> >> Probably. As noted, it's all becoming a bit unreadable with too >> much negative logic in a long conditional, so I want to clean that >> up in a follow-up. >> >>> For TYPE_UNSIGNED, do you actually need any restriction? >>> What kind of bump values are wrong for unsigned types and why? >> >> If the value of the bump is actually larger than the precision of the >> type (not likely except for quite small types), say 2 * (maxval + 1) >> which is congruent to 0, the replacement is wrong. > > Ah, ok. Anyway, for unsigned type, perhaps it could be written as: > && (TYPE_UNSIGNED (target_type) > ? wi::eq_p (wi::neg_p (bump) ? bump + wi::to_widest (maxval) + 1 > : bump, wi::zext (bump, prec)) > : wi::eq_p (bump, wi::sext (bump, prec))) > I mean, if bump >= 0, then the bump + wi::to_widest (maxval) + 1 > value has no chance to be equal to zero extended bump, and > for bump < 0 only that one has a chance. Yeah, that's true. And arguably my case for the really large bump causing problems is kind of thin, because the program is probably already broken in that case anyway. But I think I will sleep better having the check in there, as somebody other than SLSR will catch the bug then. ;-) Thanks for all the help with this one. These corner cases are always tricky, and I appreciate the extra eyeballs. Bill > > Jakub >
Re: [PATCH] Fix PR81503 (SLSR invalid fold)
On Thu, Aug 03, 2017 at 11:29:44AM -0500, Bill Schmidt wrote: > > And, wouldn't it be more readable to use: > > && (TYPE_UNSIGNED (target_type) > > ? (wi::eq_p (bump, wi::zext (bump, prec)) > > || wi::eq_p (bump + wi::to_widest (maxval) + 1, > > wi::zext (bump, prec))) > > : wi::eq_p (bump, wi::sext (bump, prec))) > > ? > > Probably. As noted, it's all becoming a bit unreadable with too > much negative logic in a long conditional, so I want to clean that > up in a follow-up. > > > For TYPE_UNSIGNED, do you actually need any restriction? > > What kind of bump values are wrong for unsigned types and why? > > If the value of the bump is actually larger than the precision of the > type (not likely except for quite small types), say 2 * (maxval + 1) > which is congruent to 0, the replacement is wrong. Ah, ok. Anyway, for unsigned type, perhaps it could be written as: && (TYPE_UNSIGNED (target_type) ? wi::eq_p (wi::neg_p (bump) ? bump + wi::to_widest (maxval) + 1 : bump, wi::zext (bump, prec)) : wi::eq_p (bump, wi::sext (bump, prec))) I mean, if bump >= 0, then the bump + wi::to_widest (maxval) + 1 value has no chance to be equal to zero extended bump, and for bump < 0 only that one has a chance. Jakub
Re: [PATCH] Fix PR81503 (SLSR invalid fold)
On Aug 3, 2017, at 11:20 AM, Jakub Jelinekwrote: > > On Thu, Aug 03, 2017 at 11:05:54AM -0500, Bill Schmidt wrote: >> --- gcc/gimple-ssa-strength-reduction.c (revision 250791) >> +++ gcc/gimple-ssa-strength-reduction.c (working copy) >> @@ -2074,6 +2074,10 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ >> { >> tree target_type = TREE_TYPE (gimple_assign_lhs (c->cand_stmt)); >> enum tree_code cand_code = gimple_assign_rhs_code (c->cand_stmt); >> + unsigned int prec = TYPE_PRECISION (target_type); >> + tree maxval = (POINTER_TYPE_P (target_type) >> + ? TYPE_MAX_VALUE (sizetype) >> + : TYPE_MAX_VALUE (target_type)); >> >> /* It is highly unlikely, but possible, that the resulting >> bump doesn't fit in a HWI. Abandon the replacement >> @@ -2082,6 +2086,17 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ >> types but allows for safe negation without twisted logic. */ >> if (wi::fits_shwi_p (bump) >> && bump.to_shwi () != HOST_WIDE_INT_MIN >> + /* It is more likely that the bump doesn't fit in the target >> + type, so check whether constraining it to that type changes >> + the value. For a signed type, the value mustn't change. >> + For an unsigned type, the value may only change to a >> + congruent value (for negative bumps). */ >> + && (TYPE_UNSIGNED (target_type) >> + || wi::eq_p (bump, wi::ext (bump, prec, SIGNED))) >> + && (!TYPE_UNSIGNED (target_type) >> + || wi::eq_p (bump, wi::ext (bump, prec, UNSIGNED)) >> + || wi::eq_p (bump + wi::to_widest (maxval) + 1, >> + wi::ext (bump, prec, UNSIGNED))) > > wi::ext makes only sense if used with variable TYPE_SIGNED, when > used with constant, it is better to use wi::sext or wi::zext. Okay. > And, wouldn't it be more readable to use: > && (TYPE_UNSIGNED (target_type) > ? (wi::eq_p (bump, wi::zext (bump, prec)) >|| wi::eq_p (bump + wi::to_widest (maxval) + 1, > wi::zext (bump, prec))) > : wi::eq_p (bump, wi::sext (bump, prec))) > ? Probably. As noted, it's all becoming a bit unreadable with too much negative logic in a long conditional, so I want to clean that up in a follow-up. > For TYPE_UNSIGNED, do you actually need any restriction? > What kind of bump values are wrong for unsigned types and why? If the value of the bump is actually larger than the precision of the type (not likely except for quite small types), say 2 * (maxval + 1) which is congruent to 0, the replacement is wrong. Bill > > Jakub >
Re: [PATCH] Fix PR81503 (SLSR invalid fold)
On Thu, Aug 03, 2017 at 11:05:54AM -0500, Bill Schmidt wrote: > --- gcc/gimple-ssa-strength-reduction.c (revision 250791) > +++ gcc/gimple-ssa-strength-reduction.c (working copy) > @@ -2074,6 +2074,10 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ > { >tree target_type = TREE_TYPE (gimple_assign_lhs (c->cand_stmt)); >enum tree_code cand_code = gimple_assign_rhs_code (c->cand_stmt); > + unsigned int prec = TYPE_PRECISION (target_type); > + tree maxval = (POINTER_TYPE_P (target_type) > + ? TYPE_MAX_VALUE (sizetype) > + : TYPE_MAX_VALUE (target_type)); > >/* It is highly unlikely, but possible, that the resulting > bump doesn't fit in a HWI. Abandon the replacement > @@ -2082,6 +2086,17 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ > types but allows for safe negation without twisted logic. */ >if (wi::fits_shwi_p (bump) >&& bump.to_shwi () != HOST_WIDE_INT_MIN > + /* It is more likely that the bump doesn't fit in the target > + type, so check whether constraining it to that type changes > + the value. For a signed type, the value mustn't change. > + For an unsigned type, the value may only change to a > + congruent value (for negative bumps). */ > + && (TYPE_UNSIGNED (target_type) > + || wi::eq_p (bump, wi::ext (bump, prec, SIGNED))) > + && (!TYPE_UNSIGNED (target_type) > + || wi::eq_p (bump, wi::ext (bump, prec, UNSIGNED)) > + || wi::eq_p (bump + wi::to_widest (maxval) + 1, > +wi::ext (bump, prec, UNSIGNED))) wi::ext makes only sense if used with variable TYPE_SIGNED, when used with constant, it is better to use wi::sext or wi::zext. And, wouldn't it be more readable to use: && (TYPE_UNSIGNED (target_type) ? (wi::eq_p (bump, wi::zext (bump, prec)) || wi::eq_p (bump + wi::to_widest (maxval) + 1, wi::zext (bump, prec))) : wi::eq_p (bump, wi::sext (bump, prec))) ? For TYPE_UNSIGNED, do you actually need any restriction? What kind of bump values are wrong for unsigned types and why? Jakub
[PATCH] Fix PR81503 (SLSR invalid fold)
Hi, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81503 identifies a problem in SLSR where an invalid replacement is made because the desired value doesn't fit in the target type. This patch addresses that issue. The signed case is simple, as we require the value not to change when restricted to the precision of the target type. For the unsigned case, a negative value is expected to be converted to the congruent positive value, so we need to check for that. We have several test cases in the test suite that exercise the negative bump for an unsigned/pointer type. Per Jakub's suggestion I attempted to do all of this within the wide_int and widest_int framework. However, I was unable to find a way to generate maxval as a widest_int within that framework. I can use wide_int maxval = wi::max_value (prec, sign); to get a wide_int, but I can't find a way to convert this to a widest_int so that I can add it to directly to bump. I am probably missing something obvious here, so I welcome any suggestions. At any rate, using TYPE_MAX_VALUE works, it's just not as clean as I would like. BTW, I plan a follow-up patch to rewrite the complex condition for executing the main block of code, which is starting to get out of control. It will be simpler to read if the conditions are negated and used to do an immediate return. Bootstrapped and tested on powerpc64le-linux-gnu with no regressions. Is this ok for trunk? (Just noticed a tab/space problem on the comment lines, which I will fix.) Thanks, Bill [gcc] 2017-08-03 Bill SchmidtPR tree-optimization/81503 * gimple-ssa-strength-reduction.c (replace_mult_candidate): Ensure folded constant fits in the target type. [gcc/testsuite] 2017-08-03 Bill Schmidt PR tree-optimization/81503 * gcc.c-torture/execute/pr81503.c: New file. Index: gcc/gimple-ssa-strength-reduction.c === --- gcc/gimple-ssa-strength-reduction.c (revision 250791) +++ gcc/gimple-ssa-strength-reduction.c (working copy) @@ -2074,6 +2074,10 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ { tree target_type = TREE_TYPE (gimple_assign_lhs (c->cand_stmt)); enum tree_code cand_code = gimple_assign_rhs_code (c->cand_stmt); + unsigned int prec = TYPE_PRECISION (target_type); + tree maxval = (POINTER_TYPE_P (target_type) +? TYPE_MAX_VALUE (sizetype) +: TYPE_MAX_VALUE (target_type)); /* It is highly unlikely, but possible, that the resulting bump doesn't fit in a HWI. Abandon the replacement @@ -2082,6 +2086,17 @@ replace_mult_candidate (slsr_cand_t c, tree basis_ types but allows for safe negation without twisted logic. */ if (wi::fits_shwi_p (bump) && bump.to_shwi () != HOST_WIDE_INT_MIN + /* It is more likely that the bump doesn't fit in the target +type, so check whether constraining it to that type changes +the value. For a signed type, the value mustn't change. + For an unsigned type, the value may only change to a + congruent value (for negative bumps). */ + && (TYPE_UNSIGNED (target_type) + || wi::eq_p (bump, wi::ext (bump, prec, SIGNED))) + && (!TYPE_UNSIGNED (target_type) + || wi::eq_p (bump, wi::ext (bump, prec, UNSIGNED)) + || wi::eq_p (bump + wi::to_widest (maxval) + 1, + wi::ext (bump, prec, UNSIGNED))) /* It is not useful to replace casts, copies, negates, or adds of an SSA name and a constant. */ && cand_code != SSA_NAME Index: gcc/testsuite/gcc.c-torture/execute/pr81503.c === --- gcc/testsuite/gcc.c-torture/execute/pr81503.c (nonexistent) +++ gcc/testsuite/gcc.c-torture/execute/pr81503.c (working copy) @@ -0,0 +1,15 @@ +unsigned short a = 41461; +unsigned short b = 3419; +int c = 0; + +void foo() { + if (a + b * ~(0 != 5)) +c = -~(b * ~(0 != 5)) + 2147483647; +} + +int main() { + foo(); + if (c != 2147476810) +return -1; + return 0; +}