This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix PR81503 (SLSR invalid fold)
- From: Bill Schmidt <wschmidt at linux dot vnet dot ibm dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Richard Biener <richard dot guenther at gmail dot com>
- Date: Thu, 3 Aug 2017 11:29:44 -0500
- Subject: Re: [PATCH] Fix PR81503 (SLSR invalid fold)
- Authentication-results: sourceware.org; auth=none
- References: <7bd4dcbb-cce0-82bb-b938-ffd85dd0e72a@linux.vnet.ibm.com> <20170803162022.GB2123@tucnak>
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
>