On Thu, Aug 3, 2017 at 9:34 PM, Bill Schmidt
<wschm...@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 <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)))
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);
?
> /* 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
>>>
>
