> On Aug 29, 2017, at 4:24 AM, Richard Biener <richard.guent...@gmail.com>
> wrote:
>
> 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
Right, I was initially concerned about that, but I believe we're ok. Since the
code was
first written, the wide-int C++ class came along and bump became a widest_int.
So
when signed MIN gets negated it will be a positive number outside the range of
an HWI.
The check for fitting in the signed target type should then take care of
overflow. The
net is that we no longer need a separate check, which is much cleaner.
>
>> + 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?
Yes, it was. It's been a long time, so I'm not sure exactly which test covers
this;
but I've always added test cases for these corner cases as they arise, and the
test suite is clean with the patch.
>
> Patch is ok. Cleanups are fine to backport if they are obvious (the
> patch doesn't tell that ;))
Yeah, that didn't diff nicely *at all*. Sorry about that.
> Please wait a few days with backporting though.
Thanks! Will do.
Bill
>
> 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;
>> +}
>>
>
