free is a killer

[Richard Biener]

On Mon, Oct 28, 2013 at 11:05 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Mon, 28 Oct 2013, Jeff Law wrote:
>
>> On 10/26/13 01:15, Marc Glisse wrote:
>>>
>>> Hello,
>>>
>>> this patch teaches gcc that free kills the memory its argument points
>>> to. The equality test is probably too strict, I guess we can loosen it
>>> later (unless you have suggestions?).
>>>
>>> Note that the corresponding code for BUILT_IN_MEMCPY and others seems
>>> suspicious to me, it looks like it is testing for equality between a
>>> pointer and a mem_ref, which is unlikely to happen.
>>>
>>> Bootstrap+testsuite on x86_64-unknown-linux-gnu.
>>>
>>> 2013-10-27  Marc Glisse  <marc.glisse@inria.fr>
>>>
>>>      PR tree-optimization/19831
>>> gcc/
>>>      * tree-ssa-alias.c (stmt_kills_ref_p_1): Handle BUILT_IN_FREE.
>>>
>>> gcc/testsuite/
>>>      * gcc.dg/tree-ssa/alias-25.c: New file.
>>
>> OK for the trunk.
>
>
> Thanks.
>
>
>> I agree the MEM_REF* and VA_END cases look strange and I think they're
>> wrong as well.  Did you happen to try fixing them and running the testsuite
>> to see if anything fails?
>>
>> ISTM your testcase could be tweaked to test conclusively if they're doing
>> the right thing -- and regardless of what's found that test can/should make
>> its way into the testsuite.

The VA_END case looks ok (though == equality testing is a bit
too conservative, the SSA_NAME case of the builtins handling
looks wrong indeed.

> I checked and it does the wrong thing (I don't have the testcase handy
> anymore, but it shouldn't be hard to recreate one), I even wrote a patch
> (attached) but it is related to:
> http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02238.html
> so it can't go in. A more limited fix (still missing many cases) would be
> possible. I didn't test VA_END, but it doesn't look as bad (it is the
> SSA_NAME case that is wrong for memcpy).
>
>
> While I am posting non-submitted patches, does the following vaguely make
> sense? I couldn't find a testcase that exercised it without some local
> patches, but the idea (IIRC) is that with:
> struct A { struct B b; int i; }
> we can easily end up with one ao_ref.base that is a MEM_REF[p] and another
> one a MEM_REF[(B*)q] where p and q are of type A*, and that prevents us from
> noticing that p->i and q->b don't alias. Maybe I should instead find a way
> to get MEM_REF[q] as ao_ref.base, but if q actually points to a larger
> structure that starts with A it is likely to fail as well...
>
> --- gcc/tree-ssa-alias.c        (revision 204095)
> +++ gcc/tree-ssa-alias.c        (working copy)
> @@ -1099,22 +1099,24 @@ indirect_refs_may_alias_p (tree ref1 ATT
>
>    /* If both references are through the same type, they do not alias
>       if the accesses do not overlap.  This does extra disambiguation
>       for mixed/pointer accesses but requires strict aliasing.  */
>    if ((TREE_CODE (base1) != TARGET_MEM_REF
>         || (!TMR_INDEX (base1) && !TMR_INDEX2 (base1)))
>        && (TREE_CODE (base2) != TARGET_MEM_REF
>           || (!TMR_INDEX (base2) && !TMR_INDEX2 (base2)))
>        && same_type_for_tbaa (TREE_TYPE (base1), TREE_TYPE (ptrtype1)) == 1
>        && same_type_for_tbaa (TREE_TYPE (base2), TREE_TYPE (ptrtype2)) == 1
> -      && same_type_for_tbaa (TREE_TYPE (ptrtype1),
> -                            TREE_TYPE (ptrtype2)) == 1)
> +      && (same_type_for_tbaa (TREE_TYPE (ptrtype1),
> +                             TREE_TYPE (ptrtype2)) == 1
> +         || same_type_for_tbaa (TREE_TYPE (TREE_TYPE (ptr1)),
> +                                TREE_TYPE (TREE_TYPE (ptr2))) == 1))

No, the type of 'ptr' are not in any way relevant.

Richard.

>      return ranges_overlap_p (offset1, max_size1, offset2, max_size2);
>
>    /* Do type-based disambiguation.  */
>    if (base1_alias_set != base2_alias_set
>        && !alias_sets_conflict_p (base1_alias_set, base2_alias_set))
>      return false;
>
>    /* Do access-path based disambiguation.  */
>    if (ref1 && ref2
>        && (handled_component_p (ref1) || handled_component_p (ref2))
>
>
>
> In the category "ugly hack that I won't submit", let me also attach this
> patch that sometimes helps notice that malloc doesn't alias other things (I
> don't know if the first hunk ever fires).
>
> --
> Marc Glisse