[PATCH][PR65823] Fix va_arg ap_copy nop detection

[PATCH][PR65823] Fix va_arg ap_copy nop detection

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 2106 bytes --]

Hi,

this patch fixes PR65823.

The problem is a verify_gimple ICE during compilation of 
gcc.c-torture/execute/stdarg-2.c for arm at -O0/-O1:
...
In function 'f3':
src/gcc/testsuite/gcc.c-torture/execute/stdarg-2.c:61:1:
error: incorrect sharing of tree nodes
aps[4]
# .MEM_5 = VDEF <.MEM_11>
aps[4] = aps[4];
...


Before gimplification, f3 looks like this in the original dump:
...
   struct va_list aps[10];

     struct va_list aps[10];
   __builtin_va_start ((struct  &) (struct  *) &aps[4], i);
   x = VA_ARG_EXPR <aps[4]>;
   __builtin_va_end ((struct  &) (struct  *) &aps[4]);
...

After gimplification, it looks like:
...
f3 (int i)
{
   long intD.5 x.0D.4231;
   struct va_listD.4222 apsD.4227[10];

   try
     {
       # USE = anything
       # CLB = anything
       __builtin_va_startD.1052 (&apsD.4227[4], 0);
       # USE = anything
       # CLB = anything
       x.0D.4231 = VA_ARG (&apsD.4227[4], 0B);
       apsD.4227[4] = apsD.4227[4];
       xD.4223 = x.0D.4231;
       # USE = anything
       # CLB = anything
       __builtin_va_endD.1051 (&apsD.4227[4]);
     }
   finally
     {
       apsD.4227 = {CLOBBER};
     }
}
...

The nop 'apsD.4227[4] = apsD.4227[4]' introduced during gimplification is not 
meant to be there.


There is already a test 'TREE_OPERAND (ap, 0) != TREE_OPERAND (ap_copy, 0))' in 
gimplify_modify_expr to prevent this nop:
...
   /* When gimplifying the &ap argument of va_arg, we might end up with
        ap.1 = ap
        va_arg (&ap.1, 0B)
      We need to assign ap.1 back to ap, otherwise va_arg has no effect on
      ap.  */
   if (ap != NULL_TREE
       && TREE_CODE (ap) == ADDR_EXPR
       && TREE_CODE (ap_copy) == ADDR_EXPR
       && TREE_OPERAND (ap, 0) != TREE_OPERAND (ap_copy, 0))
     gimplify_assign (TREE_OPERAND (ap, 0), TREE_OPERAND (ap_copy, 0), pre_p);
...

But the test is a pointer equality test, and it fails in this case.

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?

Thanks,
- Tom

[-- Attachment #2: 0001-Fix-va_arg-ap_copy-nop-detection.patch --]
[-- Type: text/x-patch, Size: 862 bytes --]

Fix va_arg ap_copy nop detection

2015-04-22  Tom de Vries  <tom@codesourcery.com>

	PR tree-optimization/65823
	* gimplify.c (gimplify_modify_expr): Use operand_equal_p to test for
	equality between ap_copy and ap.
---
 gcc/gimplify.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 0a8ef84..c68bd47 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -4792,7 +4792,7 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
   if (ap != NULL_TREE
       && TREE_CODE (ap) == ADDR_EXPR
       && TREE_CODE (ap_copy) == ADDR_EXPR
-      && TREE_OPERAND (ap, 0) != TREE_OPERAND (ap_copy, 0))
+      && !operand_equal_p (TREE_OPERAND (ap, 0), TREE_OPERAND (ap_copy, 0), 0))
     gimplify_assign (TREE_OPERAND (ap, 0), TREE_OPERAND (ap_copy, 0), pre_p);
 
   if (want_value)
-- 
1.9.1

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

[Richard Biener]

On Wed, Apr 22, 2015 at 9:41 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> Hi,
>
> this patch fixes PR65823.
>
> The problem is a verify_gimple ICE during compilation of
> gcc.c-torture/execute/stdarg-2.c for arm at -O0/-O1:
> ...
> In function 'f3':
> src/gcc/testsuite/gcc.c-torture/execute/stdarg-2.c:61:1:
> error: incorrect sharing of tree nodes
> aps[4]
> # .MEM_5 = VDEF <.MEM_11>
> aps[4] = aps[4];
> ...
>
>
> Before gimplification, f3 looks like this in the original dump:
> ...
>   struct va_list aps[10];
>
>     struct va_list aps[10];
>   __builtin_va_start ((struct  &) (struct  *) &aps[4], i);
>   x = VA_ARG_EXPR <aps[4]>;
>   __builtin_va_end ((struct  &) (struct  *) &aps[4]);
> ...
>
> After gimplification, it looks like:
> ...
> f3 (int i)
> {
>   long intD.5 x.0D.4231;
>   struct va_listD.4222 apsD.4227[10];
>
>   try
>     {
>       # USE = anything
>       # CLB = anything
>       __builtin_va_startD.1052 (&apsD.4227[4], 0);
>       # USE = anything
>       # CLB = anything
>       x.0D.4231 = VA_ARG (&apsD.4227[4], 0B);
>       apsD.4227[4] = apsD.4227[4];
>       xD.4223 = x.0D.4231;
>       # USE = anything
>       # CLB = anything
>       __builtin_va_endD.1051 (&apsD.4227[4]);
>     }
>   finally
>     {
>       apsD.4227 = {CLOBBER};
>     }
> }
> ...
>
> The nop 'apsD.4227[4] = apsD.4227[4]' introduced during gimplification is
> not meant to be there.
>
>
> There is already a test 'TREE_OPERAND (ap, 0) != TREE_OPERAND (ap_copy, 0))'
> in gimplify_modify_expr to prevent this nop:
> ...
>   /* When gimplifying the &ap argument of va_arg, we might end up with
>        ap.1 = ap
>        va_arg (&ap.1, 0B)
>      We need to assign ap.1 back to ap, otherwise va_arg has no effect on
>      ap.  */
>   if (ap != NULL_TREE
>       && TREE_CODE (ap) == ADDR_EXPR
>       && TREE_CODE (ap_copy) == ADDR_EXPR
>       && TREE_OPERAND (ap, 0) != TREE_OPERAND (ap_copy, 0))
>     gimplify_assign (TREE_OPERAND (ap, 0), TREE_OPERAND (ap_copy, 0),
> pre_p);
> ...
>
> But the test is a pointer equality test, and it fails in this case.
>
> 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.  But I wonder if we can't fix things to not require that
odd extra copy.  In fact that we introduce ap.1 looks completely bogus to me
(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"?

Thanks,
Richard.

> Thanks,
> - Tom

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

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 2169 bytes --]

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?

Thanks,
- Tom

[-- Attachment #2: 0001-Add-copy-for-va_list-parameter.patch --]
[-- Type: text/x-patch, Size: 3393 bytes --]

Add copy for va_list parameter

---
 gcc/function.c | 29 +++++++++++++++++++++++++++++
 gcc/gimplify.c | 16 ----------------
 2 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/gcc/function.c b/gcc/function.c
index 7d4df92..2ebfec4 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -3855,6 +3855,24 @@ gimplify_parm_type (tree *tp, int *walk_subtrees, void *data)
   return NULL;
 }
 
+static inline bool
+is_va_list_type (tree type)
+{
+  tree id = TYPE_IDENTIFIER (type);
+  if (id == NULL_TREE)
+    return false;
+  const char *s = IDENTIFIER_POINTER (id);
+  if (s == NULL)
+    return false;
+  if (strcmp (s, "va_list") == 0)
+    return true;
+  if (strcmp (s, "__builtin_sysv_va_list") == 0)
+    return true;
+  if (strcmp (s, "__builtin_ms_va_list") == 0)
+    return true;
+  return false;
+}
+
 /* Gimplify the parameter list for current_function_decl.  This involves
    evaluating SAVE_EXPRs of variable sized parameters and generating code
    to implement callee-copies reference parameters.  Returns a sequence of
@@ -3953,6 +3971,17 @@ gimplify_parameters (void)
 	      DECL_HAS_VALUE_EXPR_P (parm) = 1;
 	    }
 	}
+      else if (is_va_list_type (TREE_TYPE (parm)))
+	{
+	  tree cp = create_tmp_reg (data.nominal_type, get_name (parm));
+	  DECL_IGNORED_P (cp) = 0;
+	  TREE_ADDRESSABLE (cp) = 1;
+	  tree t = build2 (MODIFY_EXPR, TREE_TYPE (cp), cp, parm);
+	  gimplify_and_add (t, &stmts);
+
+	  SET_DECL_VALUE_EXPR (parm, cp);
+	  DECL_HAS_VALUE_EXPR_P (parm) = 1;
+	}
     }
 
   fnargs.release ();
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 5f1dd1a..c922dc7 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -4569,7 +4569,6 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
   gimple assign;
   location_t loc = EXPR_LOCATION (*expr_p);
   gimple_stmt_iterator gsi;
-  tree ap = NULL_TREE, ap_copy = NULL_TREE;
 
   gcc_assert (TREE_CODE (*expr_p) == MODIFY_EXPR
 	      || TREE_CODE (*expr_p) == INIT_EXPR);
@@ -4730,16 +4729,12 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 	  enum internal_fn ifn = CALL_EXPR_IFN (*from_p);
 	  auto_vec<tree> vargs (nargs);
 
-	  if (ifn == IFN_VA_ARG)
-	    ap = unshare_expr (CALL_EXPR_ARG (*from_p, 0));
 	  for (i = 0; i < nargs; i++)
 	    {
 	      gimplify_arg (&CALL_EXPR_ARG (*from_p, i), pre_p,
 			    EXPR_LOCATION (*from_p));
 	      vargs.quick_push (CALL_EXPR_ARG (*from_p, i));
 	    }
-	  if (ifn == IFN_VA_ARG)
-	    ap_copy = CALL_EXPR_ARG (*from_p, 0);
 	  call_stmt = gimple_build_call_internal_vec (ifn, vargs);
 	  gimple_set_location (call_stmt, EXPR_LOCATION (*expr_p));
 	}
@@ -4784,17 +4779,6 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
   gsi = gsi_last (*pre_p);
   maybe_fold_stmt (&gsi);
 
-  /* When gimplifying the &ap argument of va_arg, we might end up with
-       ap.1 = ap
-       va_arg (&ap.1, 0B)
-     We need to assign ap.1 back to ap, otherwise va_arg has no effect on
-     ap.  */
-  if (ap != NULL_TREE
-      && TREE_CODE (ap) == ADDR_EXPR
-      && TREE_CODE (ap_copy) == ADDR_EXPR
-      && !operand_equal_p (TREE_OPERAND (ap, 0), TREE_OPERAND (ap_copy, 0), 0))
-    gimplify_assign (TREE_OPERAND (ap, 0), TREE_OPERAND (ap_copy, 0), pre_p);
-
   if (want_value)
     {
       *expr_p = TREE_THIS_VOLATILE (*to_p) ? *from_p : unshare_expr (*to_p);
-- 
1.9.1

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

[Richard Biener]

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);

pre-approved with removing the kludge.

Thanks,
Richard.

> Thanks,
> - Tom

Re: [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

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

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 3610 bytes --]

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?

Thanks,
- Tom

[-- Attachment #2: 0001-Remove-ifn_va_arg-ap-fixup.patch --]
[-- Type: text/x-patch, Size: 2626 bytes --]

Remove ifn_va_arg ap fixup

2015-04-28  Tom de Vries  <tom@codesourcery.com>

	PR tree-optimization/65887
	* gimplify.c (gimplify_modify_expr): Remove ifn_va_arg ap fixup.

	* c-common.c (build_va_arg): Mark va_arg ap argument as addressable.
---
 gcc/c-family/c-common.c |  1 +
 gcc/gimplify.c          | 16 ----------------
 2 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 9797e17..d6a93d9 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -5910,6 +5910,7 @@ set_compound_literal_name (tree decl)
 tree
 build_va_arg (location_t loc, tree expr, tree type)
 {
+  mark_addressable (expr);
   expr = build1 (VA_ARG_EXPR, type, expr);
   SET_EXPR_LOCATION (expr, loc);
   return expr;
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index c68bd47..1d5341e 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -4569,7 +4569,6 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
   gimple assign;
   location_t loc = EXPR_LOCATION (*expr_p);
   gimple_stmt_iterator gsi;
-  tree ap = NULL_TREE, ap_copy = NULL_TREE;
 
   gcc_assert (TREE_CODE (*expr_p) == MODIFY_EXPR
 	      || TREE_CODE (*expr_p) == INIT_EXPR);
@@ -4730,16 +4729,12 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 	  enum internal_fn ifn = CALL_EXPR_IFN (*from_p);
 	  auto_vec<tree> vargs (nargs);
 
-	  if (ifn == IFN_VA_ARG)
-	    ap = unshare_expr (CALL_EXPR_ARG (*from_p, 0));
 	  for (i = 0; i < nargs; i++)
 	    {
 	      gimplify_arg (&CALL_EXPR_ARG (*from_p, i), pre_p,
 			    EXPR_LOCATION (*from_p));
 	      vargs.quick_push (CALL_EXPR_ARG (*from_p, i));
 	    }
-	  if (ifn == IFN_VA_ARG)
-	    ap_copy = CALL_EXPR_ARG (*from_p, 0);
 	  call_stmt = gimple_build_call_internal_vec (ifn, vargs);
 	  gimple_set_location (call_stmt, EXPR_LOCATION (*expr_p));
 	}
@@ -4784,17 +4779,6 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
   gsi = gsi_last (*pre_p);
   maybe_fold_stmt (&gsi);
 
-  /* When gimplifying the &ap argument of va_arg, we might end up with
-       ap.1 = ap
-       va_arg (&ap.1, 0B)
-     We need to assign ap.1 back to ap, otherwise va_arg has no effect on
-     ap.  */
-  if (ap != NULL_TREE
-      && TREE_CODE (ap) == ADDR_EXPR
-      && TREE_CODE (ap_copy) == ADDR_EXPR
-      && !operand_equal_p (TREE_OPERAND (ap, 0), TREE_OPERAND (ap_copy, 0), 0))
-    gimplify_assign (TREE_OPERAND (ap, 0), TREE_OPERAND (ap_copy, 0), pre_p);
-
   if (want_value)
     {
       *expr_p = TREE_THIS_VOLATILE (*to_p) ? *from_p : unshare_expr (*to_p);
-- 
1.9.1

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

[Richard Biener]

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