| Message ID | c3f05b38-871a-fbb2-e744-014cee2a1070@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Hi, I'd like to ping this patch, please. Thanks! Bill > On Aug 3, 2017, at 2:34 PM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> 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 <wschmidt@linux.vnet.ibm.com> > Jakub Jelinek <jakub@redhat.com> > > 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 <wschmidt@linux.vnet.ibm.com> > Jakub Jelinek <jakub@redhat.com> > > 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 <jakub@redhat.com> 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 >>> >
Ping. Thanks! Bill > On Aug 14, 2017, at 9:32 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote: > > Hi, > > I'd like to ping this patch, please. > > Thanks! > Bill > >> On Aug 3, 2017, at 2:34 PM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> 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 <wschmidt@linux.vnet.ibm.com> >> Jakub Jelinek <jakub@redhat.com> >> >> 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 <wschmidt@linux.vnet.ibm.com> >> Jakub Jelinek <jakub@redhat.com> >> >> 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 <jakub@redhat.com> 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 >>>> >> >
On Thu, Aug 3, 2017 at 9:34 PM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> 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 <wschmidt@linux.vnet.ibm.com> > Jakub Jelinek <jakub@redhat.com> > > 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 <wschmidt@linux.vnet.ibm.com> > Jakub Jelinek <jakub@redhat.com> > > 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 != 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 <jakub@redhat.com> 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 >>> >
On Aug 28, 2017, at 7:37 AM, Richard Biener <richard.guenther@gmail.com> wrote: > > On Thu, Aug 3, 2017 at 9:34 PM, Bill Schmidt > <wschmidt@linux.vnet.ibm.com> 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 <wschmidt@linux.vnet.ibm.com> >> Jakub Jelinek <jakub@redhat.com> >> >> 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 <wschmidt@linux.vnet.ibm.com> >> Jakub Jelinek <jakub@redhat.com> >> >> 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); > > ? Ah, I see what you're going for. It looks reasonable on the surface. Let me do some testing and think about it a little more. Thanks! Bill > >> /* 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 <jakub@redhat.com> 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
> On Aug 28, 2017, at 12:57 PM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote: > > On Aug 28, 2017, at 7:37 AM, Richard Biener <richard.guenther@gmail.com> wrote: >> >> On Thu, Aug 3, 2017 at 9:34 PM, Bill Schmidt >> <wschmidt@linux.vnet.ibm.com> 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 <wschmidt@linux.vnet.ibm.com> >>> Jakub Jelinek <jakub@redhat.com> >>> >>> 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 <wschmidt@linux.vnet.ibm.com> >>> Jakub Jelinek <jakub@redhat.com> >>> >>> 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); >> >> ? > > Ah, I see what you're going for. It looks reasonable on the surface. Let me do > some testing and think about it a little more. I think you still need the specific test for whether the original bump fits in an HWI, since wide_int_to_tree will convert to a tree that only stores a single HWI, right? I'll test with that remaining in place but otherwise follow the direction of your suggestion. Bill > > Thanks! > Bill > >> >>> /* 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 <jakub@redhat.com> 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
On Aug 28, 2017, at 1:40 PM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote: > >> >> On Aug 28, 2017, at 12:57 PM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote: >> >> On Aug 28, 2017, at 7:37 AM, Richard Biener <richard.guenther@gmail.com> wrote: >>> >>> On Thu, Aug 3, 2017 at 9:34 PM, Bill Schmidt >>> <wschmidt@linux.vnet.ibm.com> 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 <wschmidt@linux.vnet.ibm.com> >>>> Jakub Jelinek <jakub@redhat.com> >>>> >>>> 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 <wschmidt@linux.vnet.ibm.com> >>>> Jakub Jelinek <jakub@redhat.com> >>>> >>>> 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); >>> >>> ? >> >> Ah, I see what you're going for. It looks reasonable on the surface. Let me do >> some testing and think about it a little more. > > I think you still need the specific test for whether the original bump fits in an > HWI, since wide_int_to_tree will convert to a tree that only stores a single > HWI, right? I'll test with that remaining in place but otherwise follow the > direction of your suggestion. Mm, I'll take that back. That's only if the target type requires no more than a single HWI, so that's already covered. I think what you have is probably correct. Bill > > Bill > >> >> Thanks! >> Bill >> >>> >>>> /* 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 <jakub@redhat.com> 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
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; +}