Message ID | 7bd4dcbb-cce0-82bb-b938-ffd85dd0e72a@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
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
On Aug 3, 2017, at 11:20 AM, Jakub Jelinek <jakub@redhat.com> wrote: > > 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 >
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
-- Bill Bill Schmidt, Ph.D. GCC for Linux on Power Linux on Power Toolchain IBM Linux Technology Center wschmidt@linux.vnet.ibm.com > 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 (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; +}