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)
On Aug 28, 2017, at 1:40 PM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote:
>
>>
>> On Aug 28, 2017, at 12:57 PM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote:
>>
>> On Aug 28, 2017, at 7:37 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>>>
>>> On Thu, Aug 3, 2017 at 9: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)))
>>>
>>> Not sure, but would it be fixed in a similar way when writing
>>>
>>> @@ -2089,16 +2089,9 @@ replace_mult_candidate (slsr_cand_t c, t
>>> tree target_type = TREE_TYPE (gimple_assign_lhs (c->cand_stmt));
>>> enum tree_code cand_code = gimple_assign_rhs_code (c->cand_stmt);
>>>
>>> - /* It is highly unlikely, but possible, that the resulting
>>> - bump doesn't fit in a HWI. Abandon the replacement
>>> - in this case. This does not affect siblings or dependents
>>> - of C. Restriction to signed HWI is conservative for unsigned
>>> - types but allows for safe negation without twisted logic. */
>>> - if (wi::fits_shwi_p (bump)
>>> - && bump.to_shwi () != HOST_WIDE_INT_MIN
>>> - /* It is not useful to replace casts, copies, negates, or adds of
>>> - an SSA name and a constant. */
>>> - && cand_code != SSA_NAME
>>> + /* It is not useful to replace casts, copies, negates, or adds of
>>> + an SSA name and a constant. */
>>> + if (cand_code != SSA_NAME
>>> && !CONVERT_EXPR_CODE_P (cand_code)
>>> && cand_code != PLUS_EXPR
>>> && cand_code != POINTER_PLUS_EXPR
>>> @@ -2109,18 +2102,25 @@ replace_mult_candidate (slsr_cand_t c, t
>>> tree bump_tree;
>>> gimple *stmt_to_print = NULL;
>>>
>>> - /* If the basis name and the candidate's LHS have incompatible
>>> - types, introduce a cast. */
>>> - if (!useless_type_conversion_p (target_type, TREE_TYPE (basis_name)))
>>> - basis_name = introduce_cast_before_cand (c, target_type, basis_name);
>>> if (wi::neg_p (bump))
>>> {
>>> code = MINUS_EXPR;
>>> bump = -bump;
>>> }
>>> + /* It is possible that the resulting bump doesn't fit in target_type.
>>> + Abandon the replacement in this case. This does not affect
>>> + siblings or dependents of C. */
>>> + if (bump != wi::ext (bump, TYPE_PRECISION (target_type),
>>> + TYPE_SIGN (target_type)))
>>> + return;
>>>
>>> bump_tree = wide_int_to_tree (target_type, bump);
>>>
>>> + /* If the basis name and the candidate's LHS have incompatible
>>> + types, introduce a cast. */
>>> + if (!useless_type_conversion_p (target_type, TREE_TYPE (basis_name)))
>>> + basis_name = introduce_cast_before_cand (c, target_type, basis_name);
>>> +
>>> if (dump_file && (dump_flags & TDF_DETAILS))
>>> {
>>> fputs ("Replacing: ", dump_file);
>>>
>>> ?
>>
>> Ah, I see what you're going for. It looks reasonable on the surface. Let me do
>> some testing and think about it a little more.
>
> I think you still need the specific test for whether the original bump fits in an
> HWI, since wide_int_to_tree will convert to a tree that only stores a single
> HWI, right? I'll test with that remaining in place but otherwise follow the
> direction of your suggestion.
Mm, I'll take that back. That's only if the target type requires no more than
a single HWI, so that's already covered. I think what you have is probably
correct.
Bill
>
> Bill
>
>>
>> Thanks!
>> Bill
>>
>>>
>>>> /* 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