Message ID | C270ABA6-3138-4E58-B42E-EE9E75D5F97B@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
On Tue, Mar 14, 2017 at 3:20 PM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote: > >> On Mar 14, 2017, at 7:50 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote: >> >> >>> On Mar 14, 2017, at 3:57 AM, Richard Biener <richard.guenther@gmail.com> wrote: >>> >>> On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt >>> <wschmidt@linux.vnet.ibm.com> wrote: >>>> >>>> Index: gcc/tree-stdarg.c >>>> =================================================================== >>>> --- gcc/tree-stdarg.c (revision 246109) >>>> +++ gcc/tree-stdarg.c (working copy) >>>> @@ -1057,7 +1057,7 @@ expand_ifn_va_arg_1 (function *fun) >>>> types. */ >>>> gimplify_assign (lhs, expr, &pre); >>>> } >>>> - else >>>> + else if (is_gimple_addressable (expr)) >>>> gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); >>> >>> This is wrong - we lose side-effects this way and the only reason we gimplify >>> is to _not_ lose them. >>> >>> Better is sth like >>> >>> Index: gcc/tree-stdarg.c >>> =================================================================== >>> --- gcc/tree-stdarg.c (revision 246082) >>> +++ gcc/tree-stdarg.c (working copy) >>> @@ -1058,7 +1058,7 @@ expand_ifn_va_arg_1 (function *fun) >>> gimplify_assign (lhs, expr, &pre); >>> } >>> else >>> - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); >>> + gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either); >>> >>> input_location = saved_location; >>> pop_gimplify_context (NULL); >> >> OK, thanks for the explanation. Unfortunately this fails bootstrap with a failed >> assert a little later. I'll dig further. > > Looks like is_gimple_val is too restrictive for MEM_REFs, for which is_gimple_lvalue > passes. Trying this now: Hmm, it should simply gimplify to a load if it's not aggregate. Can you share a testcase where it doesn't work? > Index: gcc/tree-stdarg.c > =================================================================== > --- gcc/tree-stdarg.c (revision 246109) > +++ gcc/tree-stdarg.c (working copy) > @@ -1057,8 +1057,10 @@ expand_ifn_va_arg_1 (function *fun) > types. */ > gimplify_assign (lhs, expr, &pre); > } > + else if (is_gimple_addressable (expr)) I believe any is_gimple_addressable check is flawed. > + gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); > else > - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); > + gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_rvalue); > > input_location = saved_location; > pop_gimplify_context (NULL); > > >> >> Bill >> >
> On Mar 14, 2017, at 9:25 AM, Richard Biener <richard.guenther@gmail.com> wrote: > > On Tue, Mar 14, 2017 at 3:20 PM, Bill Schmidt > <wschmidt@linux.vnet.ibm.com> wrote: >> >>> On Mar 14, 2017, at 7:50 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote: >>> >>> >>>> On Mar 14, 2017, at 3:57 AM, Richard Biener <richard.guenther@gmail.com> wrote: >>>> >>>> On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt >>>> <wschmidt@linux.vnet.ibm.com> wrote: >>>>> >>>>> Index: gcc/tree-stdarg.c >>>>> =================================================================== >>>>> --- gcc/tree-stdarg.c (revision 246109) >>>>> +++ gcc/tree-stdarg.c (working copy) >>>>> @@ -1057,7 +1057,7 @@ expand_ifn_va_arg_1 (function *fun) >>>>> types. */ >>>>> gimplify_assign (lhs, expr, &pre); >>>>> } >>>>> - else >>>>> + else if (is_gimple_addressable (expr)) >>>>> gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); >>>> >>>> This is wrong - we lose side-effects this way and the only reason we gimplify >>>> is to _not_ lose them. >>>> >>>> Better is sth like >>>> >>>> Index: gcc/tree-stdarg.c >>>> =================================================================== >>>> --- gcc/tree-stdarg.c (revision 246082) >>>> +++ gcc/tree-stdarg.c (working copy) >>>> @@ -1058,7 +1058,7 @@ expand_ifn_va_arg_1 (function *fun) >>>> gimplify_assign (lhs, expr, &pre); >>>> } >>>> else >>>> - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); >>>> + gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either); >>>> >>>> input_location = saved_location; >>>> pop_gimplify_context (NULL); >>> >>> OK, thanks for the explanation. Unfortunately this fails bootstrap with a failed >>> assert a little later. I'll dig further. >> >> Looks like is_gimple_val is too restrictive for MEM_REFs, for which is_gimple_lvalue >> passes. Trying this now: > > Hmm, it should simply gimplify to a load if it's not aggregate. Can > you share a testcase > where it doesn't work? Your suggestion failed bootstrap in libiberty on vprintf-support.c. Compilation failed with: /home/wschmidt/gcc/build/gcc-mainline-test2-debug/gcc/xgcc -B/home/wschmidt/gcc/build/gcc-mainline-test2-debug/gcc/ -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/bin/ -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/bin/ -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/lib/ -isystem /home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/include -isystem /home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/sys-include -c -DHAVE_CONFIG_H -g -O2 -gtoggle -I. -I/home/wschmidt/gcc/gcc-mainline-test2/libiberty/../include -W -Wall -Wwrite-strings -Wc++-compat -Wstrict-prototypes -Wshadow=local -pedantic -D_GNU_SOURCE -fPIC /home/wschmidt/gcc/gcc-mainline-test2/libiberty/vprintf-support.c -o pic/vprintf-support.o The initial expression being gimplified is ADDR_EXPR (VAR_DECL (ap)). Gimplification turns this into MEM_REF (VAR_DECL (D.4274), 0), and the is_gimple_val test fails on that. Bill > >> Index: gcc/tree-stdarg.c >> =================================================================== >> --- gcc/tree-stdarg.c (revision 246109) >> +++ gcc/tree-stdarg.c (working copy) >> @@ -1057,8 +1057,10 @@ expand_ifn_va_arg_1 (function *fun) >> types. */ >> gimplify_assign (lhs, expr, &pre); >> } >> + else if (is_gimple_addressable (expr)) > > I believe any is_gimple_addressable check is flawed. OK, just wanted to try something quick and dirty before your end of day. Not sure how else to deal with this. Bill > >> + gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); >> else >> - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); >> + gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_rvalue); >> >> input_location = saved_location; >> pop_gimplify_context (NULL); >> >> >>> >>> Bill
On Mar 14, 2017, at 9:32 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote: > >> On Mar 14, 2017, at 9:25 AM, Richard Biener <richard.guenther@gmail.com> wrote: >> >>>>> Better is sth like >>>>> >>>>> Index: gcc/tree-stdarg.c >>>>> =================================================================== >>>>> --- gcc/tree-stdarg.c (revision 246082) >>>>> +++ gcc/tree-stdarg.c (working copy) >>>>> @@ -1058,7 +1058,7 @@ expand_ifn_va_arg_1 (function *fun) >>>>> gimplify_assign (lhs, expr, &pre); >>>>> } >>>>> else >>>>> - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); >>>>> + gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either); >>>>> >>>>> input_location = saved_location; >>>>> pop_gimplify_context (NULL); >>>> >>>> OK, thanks for the explanation. Unfortunately this fails bootstrap with a failed >>>> assert a little later. I'll dig further. >>> >>> Looks like is_gimple_val is too restrictive for MEM_REFs, for which is_gimple_lvalue >>> passes. Trying this now: >> >> Hmm, it should simply gimplify to a load if it's not aggregate. Can >> you share a testcase >> where it doesn't work? > > Your suggestion failed bootstrap in libiberty on vprintf-support.c. Compilation failed with: > > /home/wschmidt/gcc/build/gcc-mainline-test2-debug/gcc/xgcc -B/home/wschmidt/gcc/build/gcc-mainline-test2-debug/gcc/ -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/bin/ -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/bin/ -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/lib/ -isystem /home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/include -isystem /home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/sys-include -c -DHAVE_CONFIG_H -g -O2 -gtoggle -I. -I/home/wschmidt/gcc/gcc-mainline-test2/libiberty/../include -W -Wall -Wwrite-strings -Wc++-compat -Wstrict-prototypes -Wshadow=local -pedantic -D_GNU_SOURCE -fPIC /home/wschmidt/gcc/gcc-mainline-test2/libiberty/vprintf-support.c -o pic/vprintf-support.o > > The initial expression being gimplified is ADDR_EXPR (VAR_DECL (ap)). Gimplification > turns this into MEM_REF (VAR_DECL (D.4274), 0), and the is_gimple_val test fails on that. Reduced test case: typedef __builtin_va_list __gnuc_va_list; typedef __gnuc_va_list va_list; void foo (va_list args) { va_list ap; __builtin_va_copy (ap, args); (void)__builtin_va_arg (ap, int); __builtin_va_end(ap); }
Index: gcc/tree-stdarg.c =================================================================== --- gcc/tree-stdarg.c (revision 246109) +++ gcc/tree-stdarg.c (working copy) @@ -1057,8 +1057,10 @@ expand_ifn_va_arg_1 (function *fun) types. */ gimplify_assign (lhs, expr, &pre); } + else if (is_gimple_addressable (expr)) + gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); else - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); + gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_rvalue); input_location = saved_location; pop_gimplify_context (NULL);