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
>>

Reply via email to