[PATCH][PR65823] Fix va_arg ap_copy nop detection

[Tom de Vries]

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.

Thanks,
- Tom