On Wed, Jan 4, 2017 at 8:24 PM, Jeff Law <l...@redhat.com> wrote:
> On 01/04/2017 11:55 AM, Jeff Law wrote:
>>
>> On 12/09/2016 01:28 AM, Richard Biener wrote:
>>>
>>> On Wed, Dec 7, 2016 at 12:18 AM, Jeff Law <l...@redhat.com> wrote:
>>>>
>>>>
>>>>
>>>> So I was going through the various DSE related bugs as stumbled across
>>>> 67955.
>>>>
>>>> The basic issue in 67955 is that DSE is too simplistic when trying to
>>>> determine if two writes hit the same memory address. There are cases
>>>> were
>>>> PTA analysis can get us that information.
>>>>
>>>> The unfortunate problem is that PTA can generate a single points-to
>>>> set for
>>>> pointers to different elements in the same array. So we can only
>>>> exploit a
>>>> special case. Namely when we get a PTA singleton and the size of the
>>>> store
>>>> is the same size of the pointed-to object.
>>>>
>>>> That doesn't happen in a bootstrap, but it does happen for the
>>>> testcase in
>>>> the BZ as well as a handful of tests in the testsuite -- Tom reported 6
>>>> unique tests where it triggered, I ran my own tests where two more
>>>> were spit
>>>> out. Not huge. But the cost is low.
>>>>
>>>> All that I've done with Tom's patch is update the call into the PTA
>>>> system.
>>>> It's very clearly Tom's work.
>>>>
>>>> Bootstrapped and regression tested on x86_64-linux-gnu. Installing
>>>> on the
>>>> trunk.
>>>
>>>
>>> Just double-checked it doesn't break
>>>
>>> int foo (int *p, int b)
>>> {
>>> int *q;
>>> int i = 1;
>>> if (b)
>>> q = &i;
>>> else
>>> q = (void *)0;
>>> *q = 2;
>>> return i;
>>> }
>>
>> Right. ISTM this shouldn't have a singleton points-to set and thus
>> shouldn't change behavior. But looking at things more closely, it
>> looks like points-to-null is distinct from the points-to variables. ie,
>> we want to know if its a singleton and ! null. Thus we have to check
>> pt_solution_singleton_or_null_p (pi->pt, &pt_uid) && !pi->pt->null
>>
>> I think it's working for you because the path isolation code splits off
>> the NULL dereference path. Adding
>> -fno-isolate-erroneous-paths-dereference to the switches shows DSE2,
>> incorrectly IMHO, removing the i = 1 store.
>>
>> Thoughts?
>
> The more I think about this the more I'm sure we need to verify pt.null is
> not in the points-to set. I've taken the above testcase and added it as a
> negative test. Bootstrapped, regression tested and committed to the trunk
> along with the other minor cleanups you pointed out.
Note disabling this for pt.null == 1 makes it pretty useless given we compute
that conservatively to always 1 in points-to analysis (and only VRP ever sets
it to zero). See PTAs find_what_p_points_to. This is because PTA does
not conservatively compute whether a pointer may be NULL (all bugs, I have
partly fixed some and have an incomplete patch to fix others -- at the point
I looked into this we had no users of pt.null info and thus I decided the
extra constraints and complexity wasn't worth the compile-time/memory-use).
Without -fnon-call-exceptions removing the *0 = 2 store is IMHO ok, so we
only have to make sure to not break the exception case.
For
int foo (int *p, int b)
{
int *q;
int i = 1;
if (b)
q = &i;
else
q = (void *)0;
*q = 2;
i = 3;
return *q;
}
we remove all stores but the last store to i and the load from q (but we don't
replace q with &i here, a missed optimization if removing the other stores is
valid).
So for
int foo (int *p, int b)
{
int i = 1;
try {
int *q;
if (b)
q = &i;
else
q = (int *)0;
*q = 2;
} catch (...) {
return 0;
}
return i;
}
we do not remove the store to i with -fnon-call-exceptions. Which means we are
fine not testing for pt.null != 1.
?
Richard.
> Thanks,
>
> Jeff
>
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index d43c9bc..3a7ad9d 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,11 @@
> +2017-01-04 Jeff Law <l...@redhat.com>
> +
> + PR tree-optimizatin/67955
> + * tree-ssa-alias.c (same_addr_size_stores_p): Check offsets first.
> + Allow any SSA_VAR_P as the base objects. Use integer_zerop. Verify
> + the points-to solution does not include pt_null. Use DECL_PT_UID
> + unconditionally.
> +
> 2017-01-04 Uros Bizjak <ubiz...@gmail.com>
>
> * config/i386/i386.md (HI/SImode test with imm to QImode splitters):
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index d8ff32f..0cbc8cc 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +2017-01-03 Jeff Law <l...@redhat.com>
> +
> + PR tree-optimization/67955
> + * gcc.dg/tree-ssa/ssa-dse-28.c: New test.
> +
> 2017-01-04 Marek Polacek <pola...@redhat.com>
>
> PR c++/77545
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-28.c
> b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-28.c
> new file mode 100644
> index 0000000..d35377b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-28.c
> @@ -0,0 +1,20 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-dse-details -fexceptions
> -fnon-call-exceptions -fno-isolate-erroneous-paths-dereference" } */
> +
> +
> +int foo (int *p, int b)
> +{
> + int *q;
> + int i = 1;
> + if (b)
> + q = &i;
> + else
> + q = (void *)0;
> + *q = 2;
> + return i;
> +}
> +
> +/* { dg-final { scan-tree-dump-not "Deleted dead store" "dse1"} } */
> +/* { dg-final { scan-tree-dump-not "Deleted dead store" "dse2"} } */
> +/* { dg-final { scan-tree-dump-not "Deleted dead store" "dse3"} } */
> +
> diff --git a/gcc/tree-ssa-alias.c b/gcc/tree-ssa-alias.c
> index 0a9fc74..871fa12 100644
> --- a/gcc/tree-ssa-alias.c
> +++ b/gcc/tree-ssa-alias.c
> @@ -2326,9 +2326,13 @@ same_addr_size_stores_p (tree base1, HOST_WIDE_INT
> offset1, HOST_WIDE_INT size1,
> tree base2, HOST_WIDE_INT offset2, HOST_WIDE_INT
> size2,
> HOST_WIDE_INT max_size2)
> {
> - /* For now, just handle VAR_DECL. */
> - bool base1_obj_p = VAR_P (base1);
> - bool base2_obj_p = VAR_P (base2);
> + /* Offsets need to be 0. */
> + if (offset1 != 0
> + || offset2 != 0)
> + return false;
> +
> + bool base1_obj_p = SSA_VAR_P (base1);
> + bool base2_obj_p = SSA_VAR_P (base2);
>
> /* We need one object. */
> if (base1_obj_p == base2_obj_p)
> @@ -2356,13 +2360,9 @@ same_addr_size_stores_p (tree base1, HOST_WIDE_INT
> offset1, HOST_WIDE_INT size1,
> if (size1 != size2)
> return false;
>
> - /* Offsets need to be 0. */
> - if (offset1 != 0
> - || offset2 != 0)
> - return false;
>
> /* Check that memref is a store to pointer with singleton points-to info.
> */
> - if (!tree_int_cst_equal (TREE_OPERAND (memref, 1), integer_zero_node))
> + if (!integer_zerop (TREE_OPERAND (memref, 1)))
> return false;
> tree ptr = TREE_OPERAND (memref, 0);
> if (TREE_CODE (ptr) != SSA_NAME)
> @@ -2373,10 +2373,13 @@ same_addr_size_stores_p (tree base1, HOST_WIDE_INT
> offset1, HOST_WIDE_INT size1,
> || !pt_solution_singleton_or_null_p (&pi->pt, &pt_uid))
> return false;
>
> + /* If the solution has a singleton and NULL, then we can not
> + be sure that the two stores hit the same address. */
> + if (pi->pt.null)
> + return false;
> +
> /* Check that ptr points relative to obj. */
> - unsigned int obj_uid = (DECL_PT_UID_SET_P (obj)
> - ? DECL_PT_UID (obj)
> - : DECL_UID (obj));
> + unsigned int obj_uid = DECL_PT_UID (obj);
> if (obj_uid != pt_uid)
> return false;
>
>
