Re: [PATCH][PR65823] Fix va_arg ap_copy nop detection

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, Apr 28, 2015 at 11:10 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 27-04-15 09:45, Tom de Vries wrote:
>>
>> On 22-04-15 15:50, Richard Biener wrote:
>>>
>>> On Wed, Apr 22, 2015 at 3:38 PM, Tom de Vries <Tom_deVries@mentor.com>
>>> wrote:
>>>>
>>>> On 22-04-15 10:06, Richard Biener wrote:
>>>>>
>>>>>
>>>>> On Wed, Apr 22, 2015 at 9:41 AM, Tom de Vries <Tom_deVries@mentor.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> this patch fixes PR65823.
>>>>>>
>>>>
>>>> <SNIP>
>>>>
>>>>>>
>>>>>> The patches fixes the problem by using operand_equal_p to do the
>>>>>> equality
>>>>>> test.
>>>>>>
>>>>>>
>>>>>> Bootstrapped and reg-tested on x86_64.
>>>>>>
>>>>>> Did minimal non-bootstrap build on arm and reg-tested.
>>>>>>
>>>>>> OK for trunk?
>>>>>
>>>>>
>>>>>
>>>>> Hmm, ok for now.
>>>>
>>>>
>>>>
>>>> Committed.
>>>>
>>>>> But I wonder if we can't fix things to not require that
>>>>> odd extra copy.
>>>>
>>>>
>>>>
>>>> Agreed, that would be good.
>>>>
>>>>> In fact that we introduce ap.1 looks completely bogus to me
>>>>
>>>>
>>>>
>>>> AFAICT, it's introduced by gimplify_arg ('&argp') because argp (a
>>>> PARM_DECL)
>>>> is not addressable.
>>>>
>>>>> (and we don't in this case for arm).  Note that the pointer compare
>>>>> obviously
>>>>> fails because we unshare the expression.
>>>>>
>>>>> So ... what breaks if we simply remove this odd "fixup"?
>>>>>
>>>>
>>>> [ Originally mentioned at
>>>> https://gcc.gnu.org/ml/gcc/2015-02/msg00011.html .
>>>> ]
>>>>
>>>> I've committed gcc.target/x86_64/abi/callabi/vaarg-6.c specifically as a
>>>> minimal version of this problem.
>>>>
>>>> If we remove the ap_copy fixup, at original we have:
>>>> ...
>>>> ;; Function do_cpy2 (null)
>>>> {
>>>>    char * e;
>>>>
>>>>      char * e;
>>>>    e = VA_ARG_EXPR <argp>;
>>>>    e = VA_ARG_EXPR <argp>;
>>>>    if (e != b)
>>>>      {
>>>>        abort ();
>>>>      }
>>>> }
>>>> ...
>>>>
>>>> and after gimplify we have:
>>>> ...
>>>> do_cpy2 (char * argp)
>>>> {
>>>>    char * argp.1;
>>>>    char * argp.2;
>>>>    char * b.3;
>>>>    char * e;
>>>>
>>>>    argp.1 = argp;
>>>>    e = VA_ARG (&argp.1, 0B);
>>>>    argp.2 = argp;
>>>>    e = VA_ARG (&argp.2, 0B);
>>>>    b.3 = b;
>>>>    if (e != b.3) goto <D.1373>; else goto <D.1374>;
>>>>    <D.1373>:
>>>>    abort ();
>>>>    <D.1374>:
>>>> }
>>>> ...
>>>>
>>>> The second VA_ARG uses &argp.2, which is a copy of argp, which is
>>>> unmodified
>>>> by the first VA_ARG.
>>>>
>>>>
>>>> Using attached _proof-of-concept_ patch, I get callabi.exp working
>>>> without
>>>> the ap_copy, still at the cost of one 'argp.1 = argp' copy though:
>>>> ...
>>>> do_cpy2 (char * argp)
>>>> {
>>>>    char * argp.1;
>>>>    char * b.2;
>>>>    char * e;
>>>>
>>>>    argp.1 = argp;
>>>>    e = VA_ARG (&argp.1, 0B);
>>>>    e = VA_ARG (&argp.1, 0B);
>>>>    b.2 = b;
>>>>    if (e != b.2) goto <D.1372>; else goto <D.1373>;
>>>>    <D.1372>:
>>>>    abort ();
>>>>    <D.1373>:
>>>> }
>>>> ...
>>>>
>>>> But perhaps there's an easier way?
>>>
>>>
>>> Hum, simply
>>>
>>> Index: gcc/gimplify.c
>>> ===================================================================
>>> --- gcc/gimplify.c      (revision 222320)
>>> +++ gcc/gimplify.c      (working copy)
>>> @@ -9419,6 +9419,7 @@ gimplify_va_arg_expr (tree *expr_p, gimp
>>>       }
>>>
>>>     /* Transform a VA_ARG_EXPR into an VA_ARG internal function.  */
>>> +  mark_addressable (valist);
>>>     ap = build_fold_addr_expr_loc (loc, valist);
>>>     tag = build_int_cst (build_pointer_type (type), 0);
>>>     *expr_p = build_call_expr_internal_loc (loc, IFN_VA_ARG, type, 2, ap,
>>> tag);
>>>
>>
>> That sort of works, but causes other problems. Filed PR65887 - 'remove
>> va_arg ap
>> copies' to track this issue.
>>
>
> this patch marks the va_arg ap argument in the frontend as addressable, and
> removes the fixup.
>
> Bootstrapped and reg-tested on x86_64.
>
> OK for trunk?

Ok with adding a comment to build_va_arg that gimplification later wants to take
the address of expr.

Thanks,
Richard.

> Thanks,
> - Tom