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

[Tom de Vries <Tom_deVries at mentor dot com>]

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
...

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

Evaluate side-effects in expand_ifn_va_arg_1

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

	PR tree-optimization/65818
	* tree-stdarg.c (expand_ifn_va_arg_1): Ensure that side-effects are
	evaluated.
---
 gcc/tree-stdarg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gcc/tree-stdarg.c b/gcc/tree-stdarg.c
index 16a9e2c..1356374 100644
--- a/gcc/tree-stdarg.c
+++ b/gcc/tree-stdarg.c
@@ -1078,6 +1078,8 @@ expand_ifn_va_arg_1 (function *fun)
 	       types.  */
 	    gimplify_assign (lhs, expr, &pre);
 	  }
+	else
+	  gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
 
 	pop_gimplify_context (NULL);
 
-- 
1.9.1