`On Fri, Apr 29, 2016 at 3:15 PM, Jeff Law <l...@redhat.com> wrote:
> On 04/19/2016 11:50 AM, Patrick Palka wrote:
>
>> 1. This patch introduces a "regression" in gcc.dg/tree-ssa/ssa-thread-11.c
>> in that we no longer perform FSM threading during vrp2 but instead we
>> detect two new jump threading opportunities during vrp1. Not sure if
>> the new code is better but it is shorter. I wonder how this should be
>> resolved...
>
> Definitely not a regression. As you note we thread two jumps earlier
> utilizing your new code. With the old code we're dependent upon other
> simplifications occurring which eventually exposes the FSM threads that the
> test is checking for in vrp2.
>
> I think we just want to remove the test for FSM jump threads in VRP2. We get
> coverage for your test via ssa-thread-14. That should just leave the
> verification that we do not have irreducible loops at the end of VRP2 in
> ssa-thread-11. I'll make that change.
>
> I do see what appears to be a missed jump thread, but that's not affected
> positively or negatively by your change. There's a reasonable chance it's
> only exposed by normal thread jumps in VRP2 and since we don't iterate, it's
> left in the IL. I haven't analyzed it in detail, but my hope is that when I
> pull the backwards threading out into its own pass that we'll start picking
> up more of these secondary effects.
Interesting info. I also spotted another minor optimization
opportunity within this test case, although more related to vrp than
to jump threading. It would require iterating over the use statements
of a subexpression within a conditional, and it turns out that VRP
already does this so it's only a matter of adding another case to test
for during each iteration. I'll post a patch when it's ready.
>
>
>>
>> gcc/ChangeLog:
>>
>> * tree-ssa-threadedge.c (simplify_control_stmt_condition): Split
>> out into ...
>> (simplify_control_stmt_condition_1): ... here. Recurse into
>> BIT_AND_EXPRs and BIT_IOR_EXPRs.
>>
>> gcc/testsuite/ChangeLog:
>>
>> * gcc.dg/tree-ssa/ssa-thread-14.c: New test.
>> ---
>
> I fixed a few formatting nits (too long lines).
>
>
>> +
>> + if (res1 != NULL_TREE && res2 != NULL_TREE)
>> + {
>> + if (rhs_code == BIT_AND_EXPR
>> + && TYPE_PRECISION (TREE_TYPE (op0)) == 1
>> + && integer_nonzerop (res1)
>> + && integer_nonzerop (res2))
>> + {
>> + /* If A != 0 and B != 0 then (bool)(A | B) != 0 is true.
>> */
>> + if (cond_code == NE_EXPR)
>> + return one_cst;
>> + /* If A != 0 and B != 0 then (bool)(A | B) == 0 is
>> false. */
>> + if (cond_code == EQ_EXPR)
>> + return zero_cst;
>
> I think you wanted (A & B) in the two immediately preceding comments, which
> I fixed.
>
>
> I suspect there's other stuff we could do in this space, but you've probably
> covered the most important cases.
>
>> + /* Handle (A CMP B) CMP 0. */
>> + else if (TREE_CODE_CLASS (gimple_assign_rhs_code (def_stmt))
>> + == tcc_comparison)
>> + {
>> + tree rhs1 = gimple_assign_rhs1 (def_stmt);
>> + tree rhs2 = gimple_assign_rhs2 (def_stmt);
>> +
>> + tree_code new_cond = gimple_assign_rhs_code (def_stmt);
>> + if (cond_code == EQ_EXPR)
>> + new_cond = invert_tree_comparison (new_cond, false);
>> +
>> + tree res
>> + = simplify_control_stmt_condition_1 (e, def_stmt,
>> avail_exprs_stack,
>> + rhs1, new_cond, rhs2,
>> + dummy_cond, simplify,
>> +
>> handle_dominating_asserts,
>> + limit - 1);
>> + if (res != NULL_TREE && is_gimple_min_invariant (res))
>> + return res;
>
> I was a bit confused by this case, but then realized that we already
> narrowed COND_CODE to EQ_EXPR/NE_EXPR.
>
> I made the minor edits noted above and committed the change.
Awesome, thanks for your help.
>
> Thanks,
> Jeff
