This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[PATCH][PING^2] Fix PR81503 (SLSR invalid fold)
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
>>>>
>>
>