Re: [PATCH][PR65818][bootstrap,hppa] Return side-effect free result in gimplify_va_arg_internal

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Apr 28, 2015 at 10:29 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 28-04-15 12:34, Richard Biener wrote:
>>
>> On Mon, Apr 27, 2015 at 5:04 PM, Tom de Vries <Tom_deVries@mentor.com>
>> wrote:
>>>
>>> On 27-04-15 15:40, Richard Biener wrote:
>>>>
>>>>
>>>> On Mon, Apr 27, 2015 at 3:06 PM, Tom de Vries <Tom_deVries@mentor.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 27-04-15 10:17, Richard Biener wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> This patch fixes that by gimplifying the address expression of the
>>>>>>> mem-ref
>>>>>>>>
>>>>>>>>
>>>>>>>> returned by the target hook (borrowing code from gimplify_expr, case
>>>>>>>> MEM_REF).
>>>>>>>>
>>>>>>>> Bootstrapped and reg-tested on x86_64.
>>>>>>>>
>>>>>>>> Bootstrapped and reg-tested on hppa2.0w-hp-hpux11.11.
>>>>>>>>
>>>>>>>> OK for trunk?
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hmm, that assert looks suspicious...
>>>>>>
>>>>>> Can't you simply always do
>>>>>>
>>>>>>      gimplify_expr (expr, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> It's a bit counter-intuitive for me, using is_gimple_lvalue for
>>>>> something
>>>>> (the result of va_arg) we use as rvalue.
>>>>
>>>>
>>>>
>>>> Yeah, choosing that was done because you could assert it's a MEM_REF
>>>> and tree-stdarg eventually builds a WITH_SIZE_EXPR around it.
>>>>
>>>> It would possibly be easier to simply "inline" gimplify_va_arg_internal
>>>> at its use and only gimplify the assignment.  Though in that case the
>>>> original code probably worked - just in the lhs == NULL case it didn't,
>>>> which hints at a better place for the fix - in expand_ifn_va_arg_1 do
>>>>
>>>>    if (lhs != NULL_TREE)
>>>>      {
>>>> ...
>>>>      }
>>>>    else
>>>>      gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either);
>>>>
>>>> So ... if you can re-try that one it's pre-approved.
>>>>
>>>
>>> Using that proposed patch, we run into the following problem.
>>>
>>> Consider this testcase with ignored-result-va_arg:
>>> ...
>>> #include <stdarg.h>
>>>
>>> void
>>> f2 (int i, ...)
>>> {
>>>    va_list ap;
>>>    va_start (ap, i);
>>>    va_arg (ap, long);
>>>    va_end (ap);
>>> }
>>> ...
>>>
>>> after gimplify_va_arg_internal we have:
>>> ...
>>> (gdb) call debug_generic_expr (expr)
>>> *(long int * {ref-all}) addr.2
>>> ...
>>>
>>> In a bit more detail:
>>> ...
>>> (gdb) call debug_tree (expr)
>>>   <mem_ref 0x7ffff65ea1b8
>>>      type <integer_type 0x7ffff64a77e0 long int DI
>>>          size <integer_cst 0x7ffff64a3ca8 constant 64>
>>>          unit size <integer_cst 0x7ffff64a3cc0 constant 8>
>>>          align 64 symtab 0 alias set -1 canonical type 0x7ffff64a77e0
>>> precision 64 min <integer_cst 0x7ffff64a3f30 -9223372036854775808> max
>>> <integer_cst 0x7ffff64a3f48 9223372036854775807>
>>>          pointer_to_this <pointer_type 0x7ffff65e0498>>
>>>
>>>      arg 0 <nop_expr 0x7ffff65cbb60
>>>          type <pointer_type 0x7ffff65e0498 type <integer_type
>>> 0x7ffff64a77e0
>>> long int>
>>>              public static unsigned DI size <integer_cst 0x7ffff64a3ca8
>>> 64>
>>> unit size <integer_cst 0x7ffff64a3cc0 8>
>>>              align 64 symtab 0 alias set -1 canonical type
>>> 0x7ffff65e0498>
>>>
>>>          arg 0 <var_decl 0x7ffff65e32d0 addr.2 type <pointer_type
>>> 0x7ffff64c2150>
>>>              used unsigned ignored DI file stdarg-1.c line 4 col 1 size
>>> <integer_cst 0x7ffff64a3ca8 64> unit size <integer_cst 0x7ffff64a3cc0 8>
>>>              align 64 context <function_decl 0x7ffff65c21b0 f2>>>
>>>      arg 1 <integer_cst 0x7ffff65e64e0 type <pointer_type 0x7ffff65e0498>
>>> constant 0>>
>>> ...
>>>
>>> During 'gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either)' we
>>> ICE
>>> here:
>>> ...
>>> 8886      gcc_assert ((*gimple_test_f) (*expr_p));
>>> ...
>>>
>>> At this point expr is:
>>> ...
>>> (gdb) call debug_generic_expr (*expr_p)
>>> *addr.2
>>> ...
>>>
>>> In more detail:
>>> ...
>>> (gdb) call debug_tree (*expr_p)
>>>   <mem_ref 0x7ffff65ea1e0
>>>      type <void_type 0x7ffff64c2000 void VOID
>>>          align 8 symtab 0 alias set -1 canonical type 0x7ffff64c2000
>>>          pointer_to_this <pointer_type 0x7ffff64c2150>>
>>>
>>>      arg 0 <var_decl 0x7ffff65e32d0 addr.2
>>>          type <pointer_type 0x7ffff64c2150 type <void_type 0x7ffff64c2000
>>> void>
>>>              sizes-gimplified public unsigned DI
>>>              size <integer_cst 0x7ffff64a3ca8 constant 64>
>>>              unit size <integer_cst 0x7ffff64a3cc0 constant 8>
>>>              align 64 symtab 0 alias set -1 canonical type 0x7ffff64c2150
>>>              pointer_to_this <pointer_type 0x7ffff64c9bd0>>
>>>          used unsigned ignored DI file stdarg-1.c line 4 col 1 size
>>> <integer_cst 0x7ffff64a3ca8 64> unit size <integer_cst 0x7ffff64a3cc0 8>
>>>          align 64 context <function_decl 0x7ffff65c21b0 f2>>
>>>      arg 1 <integer_cst 0x7ffff64c10c0 type <pointer_type 0x7ffff64c2150>
>>> constant 0>>
>>> ...
>>>
>>> The memref is now VOID type. And that's not a gimple_val:
>>> ...
>>> (gdb) p is_gimple_val (*expr_p)
>>> $1 = false
>>> ...
>>
>>
>> On which target?  I can't seem to reproduce on x86_64 or i?86.  I also
>> can't see how this
>> could happen.
>>
>
> Reproduced using attached patch on top of r222537, for target x86_64, with
> and without -m32.

Ok, I see now.  The gimplifier seems to be confused with fb_lvalue.
all is_gimple_addressable ()s are already is_gimple_lvalue so

Index: gcc/gimplify.c
===================================================================
--- gcc/gimplify.c      (revision 222537)
+++ gcc/gimplify.c      (working copy)
@@ -8844,15 +8844,9 @@ gimplify_expr (tree *expr_p, gimple_seq
      rvalue.  */
   if ((fallback & fb_lvalue)
       && gimple_seq_empty_p (internal_post)
-      && is_gimple_addressable (*expr_p))
-    {
-      /* An lvalue will do.  Take the address of the expression, store it
-        in a temporary, and replace the expression with an INDIRECT_REF of
-        that temporary.  */
-      tmp = build_fold_addr_expr_loc (input_location, *expr_p);
-      gimplify_expr (&tmp, pre_p, post_p, is_gimple_reg, fb_rvalue);
-      *expr_p = build_simple_mem_ref (tmp);
-    }
+      && is_gimple_lvalue (*expr_p))
+    /* An lvalue will do and we already have one.  */
+    ;
   else if ((fallback & fb_rvalue) && is_gimple_reg_rhs_or_call (*expr_p))
     {
       /* An rvalue will do.  Assign the gimplified expression into a

should be enough for that case.  OTOH is_gimple_val will not be true
after this, so we'd still ICE later.  Which means fb_lvalue doesn't make
sense for is_gimple_val.

So your patch looks ok.

Richard.

> Thanks,
> - Tom
>
>
>> Richard.
>>
>>> Attached patch uses is_gimple_lvalue, but in expand_ifn_va_arg_1.
>>>
>>> I'll bootstrap and reg-test on x86_64 and commit, unless further comments
>>> of
>>> course.
>>>
>>> Thanks,
>>> - Tom
>
>