On Mon, Aug 28, 2017 at 10:16 PM, Bill Schmidt
<wschm...@linux.vnet.ibm.com> wrote:
>> On Aug 28, 2017, at 7:37 AM, Richard Biener <richard.guent...@gmail.com>
>> wrote:
>>
>> Not sure, but would it be fixed in a similar way when writing
> <snip>
>> ?
>
> Thanks, Richard, that works very well. I decided this was a good opportunity
> to also
> simplify the control flow a little with early returns. Here's the result.
> Bootstrapped and
> tested on powerpc64le-linux-gnu with no regressions. Is this ok for trunk,
> and
> eventually for backport to gcc 5, 6, and 7? (I can omit the control flow
> cleanups for
> the older releases if desired.)
Note I removed the to_shwi () != HOST_WIDE_INT_MIN check and we might end up
negating a signed MIN value, turning it into itself but doing plus -> minus in
> + if (wi::neg_p (bump))
> {
> - enum tree_code code = PLUS_EXPR;
> - tree bump_tree;
> - gimple *stmt_to_print = NULL;
> + code = MINUS_EXPR;
> + bump = -bump;
> + }
I'm not sure if that's an issue in practice (I doubt). I think we
keep whatever the
user wrote if you write that in source in regular folding, not doing x - INT_MIN
to x + INT_MIN canonicalization. Was the != HOST_WIDE_INT_MIN check
done for any wrong-code issue?
Patch is ok. Cleanups are fine to backport if they are obvious (the
patch doesn't tell that ;))
Please wait a few days with backporting though.
Thanks,
Richard.
> Thanks,
> Bill
>
> [gcc]
>
> 2017-08-03 Bill Schmidt <wschm...@linux.vnet.ibm.com>
> Jakub Jelinek <ja...@redhat.com>
> Richard Biener <rguent...@suse.de>
>
> PR tree-optimization/81503
> * gimple-ssa-strength-reduction.c (replace_mult_candidate): Ensure
> folded constant fits in the target type; reorder tests for clarity.
>
> [gcc/testsuite]
>
> 2017-08-03 Bill Schmidt <wschm...@linux.vnet.ibm.com>
> Jakub Jelinek <ja...@redhat.com>
> Richard Biener <rguent...@suse.de>
>
> 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 251369)
> +++ gcc/gimple-ssa-strength-reduction.c (working copy)
> @@ -2089,104 +2089,104 @@ 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);
>
> - /* 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
> - && !CONVERT_EXPR_CODE_P (cand_code)
> - && cand_code != PLUS_EXPR
> - && cand_code != POINTER_PLUS_EXPR
> - && cand_code != MINUS_EXPR
> - && cand_code != NEGATE_EXPR)
> + /* 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
> + || cand_code == MINUS_EXPR
> + || cand_code == NEGATE_EXPR)
> + return;
> +
> + enum tree_code code = PLUS_EXPR;
> + tree bump_tree;
> + gimple *stmt_to_print = NULL;
> +
> + if (wi::neg_p (bump))
> {
> - enum tree_code code = PLUS_EXPR;
> - tree bump_tree;
> - gimple *stmt_to_print = NULL;
> + code = MINUS_EXPR;
> + bump = -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 (wi::neg_p (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);
> + print_gimple_stmt (dump_file, c->cand_stmt, 0);
> + }
> +
> + if (bump == 0)
> + {
> + tree lhs = gimple_assign_lhs (c->cand_stmt);
> + gassign *copy_stmt = gimple_build_assign (lhs, basis_name);
> + gimple_stmt_iterator gsi = gsi_for_stmt (c->cand_stmt);
> + slsr_cand_t cc = c;
> + gimple_set_location (copy_stmt, gimple_location (c->cand_stmt));
> + gsi_replace (&gsi, copy_stmt, false);
> + c->cand_stmt = copy_stmt;
> + while (cc->next_interp)
> {
> - code = MINUS_EXPR;
> - bump = -bump;
> + cc = lookup_cand (cc->next_interp);
> + cc->cand_stmt = copy_stmt;
> }
> -
> - bump_tree = wide_int_to_tree (target_type, bump);
> -
> if (dump_file && (dump_flags & TDF_DETAILS))
> + stmt_to_print = copy_stmt;
> + }
> + else
> + {
> + tree rhs1, rhs2;
> + if (cand_code != NEGATE_EXPR) {
> + rhs1 = gimple_assign_rhs1 (c->cand_stmt);
> + rhs2 = gimple_assign_rhs2 (c->cand_stmt);
> + }
> + if (cand_code != NEGATE_EXPR
> + && ((operand_equal_p (rhs1, basis_name, 0)
> + && operand_equal_p (rhs2, bump_tree, 0))
> + || (operand_equal_p (rhs1, bump_tree, 0)
> + && operand_equal_p (rhs2, basis_name, 0))))
> {
> - fputs ("Replacing: ", dump_file);
> - print_gimple_stmt (dump_file, c->cand_stmt, 0);
> + if (dump_file && (dump_flags & TDF_DETAILS))
> + {
> + fputs ("(duplicate, not actually replacing)", dump_file);
> + stmt_to_print = c->cand_stmt;
> + }
> }
> -
> - if (bump == 0)
> + else
> {
> - tree lhs = gimple_assign_lhs (c->cand_stmt);
> - gassign *copy_stmt = gimple_build_assign (lhs, basis_name);
> gimple_stmt_iterator gsi = gsi_for_stmt (c->cand_stmt);
> slsr_cand_t cc = c;
> - gimple_set_location (copy_stmt, gimple_location (c->cand_stmt));
> - gsi_replace (&gsi, copy_stmt, false);
> - c->cand_stmt = copy_stmt;
> + gimple_assign_set_rhs_with_ops (&gsi, code, basis_name, bump_tree);
> + update_stmt (gsi_stmt (gsi));
> + c->cand_stmt = gsi_stmt (gsi);
> while (cc->next_interp)
> {
> cc = lookup_cand (cc->next_interp);
> - cc->cand_stmt = copy_stmt;
> + cc->cand_stmt = gsi_stmt (gsi);
> }
> if (dump_file && (dump_flags & TDF_DETAILS))
> - stmt_to_print = copy_stmt;
> + stmt_to_print = gsi_stmt (gsi);
> }
> - else
> - {
> - tree rhs1, rhs2;
> - if (cand_code != NEGATE_EXPR) {
> - rhs1 = gimple_assign_rhs1 (c->cand_stmt);
> - rhs2 = gimple_assign_rhs2 (c->cand_stmt);
> - }
> - if (cand_code != NEGATE_EXPR
> - && ((operand_equal_p (rhs1, basis_name, 0)
> - && operand_equal_p (rhs2, bump_tree, 0))
> - || (operand_equal_p (rhs1, bump_tree, 0)
> - && operand_equal_p (rhs2, basis_name, 0))))
> - {
> - if (dump_file && (dump_flags & TDF_DETAILS))
> - {
> - fputs ("(duplicate, not actually replacing)", dump_file);
> - stmt_to_print = c->cand_stmt;
> - }
> - }
> - else
> - {
> - gimple_stmt_iterator gsi = gsi_for_stmt (c->cand_stmt);
> - slsr_cand_t cc = c;
> - gimple_assign_set_rhs_with_ops (&gsi, code,
> - basis_name, bump_tree);
> - update_stmt (gsi_stmt (gsi));
> - c->cand_stmt = gsi_stmt (gsi);
> - while (cc->next_interp)
> - {
> - cc = lookup_cand (cc->next_interp);
> - cc->cand_stmt = gsi_stmt (gsi);
> - }
> - if (dump_file && (dump_flags & TDF_DETAILS))
> - stmt_to_print = gsi_stmt (gsi);
> - }
> - }
> + }
>
> - if (dump_file && (dump_flags & TDF_DETAILS))
> - {
> - fputs ("With: ", dump_file);
> - print_gimple_stmt (dump_file, stmt_to_print, 0);
> - fputs ("\n", dump_file);
> - }
> + if (dump_file && (dump_flags & TDF_DETAILS))
> + {
> + fputs ("With: ", dump_file);
> + print_gimple_stmt (dump_file, stmt_to_print, 0);
> + fputs ("\n", dump_file);
> }
> }
>
> 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;
> +}
>
