| Message ID | 553E500B.2060505@mentor.com |
|---|---|
| State | New |
| Headers | show |
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. 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
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