Ping. Thanks! Bill
> On Aug 14, 2017, at 9:32 AM, Bill Schmidt <wschm...@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 <wschm...@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 <wschm...@linux.vnet.ibm.com> >> Jakub Jelinek <ja...@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 <wschm...@linux.vnet.ibm.com> >> Jakub Jelinek <ja...@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 <ja...@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 >>>> >> >