Re: [PATCH] Fix PR79908
On Mar 20, 2017, at 3:26 AM, Richard Biener wrote: > > Hmm, I think force_gimple_oeprand overwrites what is in &pre so can > you try with using a temporary sequence for force_gimple_operand and > appending that to pre afterwards instead? Indeed, that solves the problem. I'll prepare the patch for review. Thanks! Bill
Re: [PATCH] Fix PR79908
On Fri, Mar 17, 2017 at 6:27 PM, Bill Schmidt wrote: > >> On Mar 17, 2017, at 9:52 AM, Bill Schmidt >> wrote: >> >> Hi, >> >>> On Mar 17, 2017, at 6:44 AM, Richard Biener >>> wrote: >>> >>> No, I was confused in thinking gimplify_expr would handle the case >>> properly. For >>> just gimplifying side-effects we should use the middle-end >>> gimplification machinery: >>> >>> Index: tree-stdarg.c >>> === >>> --- tree-stdarg.c (revision 246188) >>> +++ tree-stdarg.c (working copy) >>> @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3. >>> #include "gimple-iterator.h" >>> #include "gimple-walk.h" >>> #include "gimplify.h" >>> +#include "gimplify-me.h" >>> #include "tree-into-ssa.h" >>> #include "tree-cfg.h" >>> #include "tree-stdarg.h" >>> @@ -1058,12 +1059,12 @@ expand_ifn_va_arg_1 (function *fun) >>> gimplify_assign (lhs, expr, &pre); >>> } >>> else >>> - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); >>> + force_gimple_operand (expr, &pre, false, NULL_TREE); >>> >>> input_location = saved_location; >>> pop_gimplify_context (NULL); >>> >>> - gimple_seq_add_seq (&pre, post); >>> + gimple_seq_add_seq_without_update (&pre, post); >>> update_modified_stmts (pre); >>> >>> /* Add the sequence after IFN_VA_ARG. This splits the bb right >>> @@ -1072,11 +1073,10 @@ expand_ifn_va_arg_1 (function *fun) >>> gimple_find_sub_bbs (pre, &i); >>> >>> /* Remove the IFN_VA_ARG gimple_call. It's the last stmt in the >>> - bb. */ >>> + bb if we added any stmts. */ >>> unlink_stmt_vdef (stmt); >>> release_ssa_name_fn (fun, gimple_vdef (stmt)); >>> gsi_remove (&i, true); >>> - gcc_assert (gsi_end_p (i)); >>> >>> /* We're walking here into the bbs which contain the expansion of >>> IFN_VA_ARG, and will not contain another IFN_VA_ARG that needs >> >> Looks good, but for some reason I hit a segfault in the linker building >> gengtype when I tried to bootstrap with this. I assume it's something >> latent and unrelated, but will need to investigate. > > Ah, I misread the build log. The link of gengtype succeeds, but gengtype > has apparently been miscompiled (stage2) as it tries to call strlen() with > an address of zero. So something wrong with this patch. Looks like the > miscompilation is of libiberty_vprintf_buffer_size in > libiberty/vprintf-support.c. > > I looked at the before and after dumps and we see that VA_ARGs with > no lhs are just removed from the code, instead of expanding the advance > of the va pointer. > > Before: > > [1.39%]: > VA_ARG (&ap, 0B, 0B); > goto (); [100.00%] > > [1.39%]: > VA_ARG (&ap, 0B, 0B); > total_width_89 = total_width_17 + 337; > goto (); [100.00%] > > After: > > [1.39%]: > goto (); [100.00%] > > [1.39%]: > total_width_89 = total_width_17 + 337; > goto (); [100.00%] Hmm, I think force_gimple_oeprand overwrites what is in &pre so can you try with using a temporary sequence for force_gimple_operand and appending that to pre afterwards instead? > Thanks, > Bill
Re: [PATCH] Fix PR79908
> On Mar 17, 2017, at 9:52 AM, Bill Schmidt wrote: > > Hi, > >> On Mar 17, 2017, at 6:44 AM, Richard Biener >> wrote: >> >> No, I was confused in thinking gimplify_expr would handle the case >> properly. For >> just gimplifying side-effects we should use the middle-end >> gimplification machinery: >> >> Index: tree-stdarg.c >> === >> --- tree-stdarg.c (revision 246188) >> +++ tree-stdarg.c (working copy) >> @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3. >> #include "gimple-iterator.h" >> #include "gimple-walk.h" >> #include "gimplify.h" >> +#include "gimplify-me.h" >> #include "tree-into-ssa.h" >> #include "tree-cfg.h" >> #include "tree-stdarg.h" >> @@ -1058,12 +1059,12 @@ expand_ifn_va_arg_1 (function *fun) >> gimplify_assign (lhs, expr, &pre); >> } >> else >> - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); >> + force_gimple_operand (expr, &pre, false, NULL_TREE); >> >> input_location = saved_location; >> pop_gimplify_context (NULL); >> >> - gimple_seq_add_seq (&pre, post); >> + gimple_seq_add_seq_without_update (&pre, post); >> update_modified_stmts (pre); >> >> /* Add the sequence after IFN_VA_ARG. This splits the bb right >> @@ -1072,11 +1073,10 @@ expand_ifn_va_arg_1 (function *fun) >> gimple_find_sub_bbs (pre, &i); >> >> /* Remove the IFN_VA_ARG gimple_call. It's the last stmt in the >> - bb. */ >> + bb if we added any stmts. */ >> unlink_stmt_vdef (stmt); >> release_ssa_name_fn (fun, gimple_vdef (stmt)); >> gsi_remove (&i, true); >> - gcc_assert (gsi_end_p (i)); >> >> /* We're walking here into the bbs which contain the expansion of >> IFN_VA_ARG, and will not contain another IFN_VA_ARG that needs > > Looks good, but for some reason I hit a segfault in the linker building > gengtype when I tried to bootstrap with this. I assume it's something > latent and unrelated, but will need to investigate. Ah, I misread the build log. The link of gengtype succeeds, but gengtype has apparently been miscompiled (stage2) as it tries to call strlen() with an address of zero. So something wrong with this patch. Looks like the miscompilation is of libiberty_vprintf_buffer_size in libiberty/vprintf-support.c. I looked at the before and after dumps and we see that VA_ARGs with no lhs are just removed from the code, instead of expanding the advance of the va pointer. Before: [1.39%]: VA_ARG (&ap, 0B, 0B); goto (); [100.00%] [1.39%]: VA_ARG (&ap, 0B, 0B); total_width_89 = total_width_17 + 337; goto (); [100.00%] After: [1.39%]: goto (); [100.00%] [1.39%]: total_width_89 = total_width_17 + 337; goto (); [100.00%] Thanks, Bill
Re: [PATCH] Fix PR79908
Hi, > On Mar 17, 2017, at 6:44 AM, Richard Biener > wrote: > > No, I was confused in thinking gimplify_expr would handle the case > properly. For > just gimplifying side-effects we should use the middle-end > gimplification machinery: > > Index: tree-stdarg.c > === > --- tree-stdarg.c (revision 246188) > +++ tree-stdarg.c (working copy) > @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3. > #include "gimple-iterator.h" > #include "gimple-walk.h" > #include "gimplify.h" > +#include "gimplify-me.h" > #include "tree-into-ssa.h" > #include "tree-cfg.h" > #include "tree-stdarg.h" > @@ -1058,12 +1059,12 @@ expand_ifn_va_arg_1 (function *fun) >gimplify_assign (lhs, expr, &pre); > } >else > - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); > + force_gimple_operand (expr, &pre, false, NULL_TREE); > >input_location = saved_location; >pop_gimplify_context (NULL); > > - gimple_seq_add_seq (&pre, post); > + gimple_seq_add_seq_without_update (&pre, post); >update_modified_stmts (pre); > >/* Add the sequence after IFN_VA_ARG. This splits the bb right > @@ -1072,11 +1073,10 @@ expand_ifn_va_arg_1 (function *fun) >gimple_find_sub_bbs (pre, &i); > >/* Remove the IFN_VA_ARG gimple_call. It's the last stmt in the > - bb. */ > + bb if we added any stmts. */ >unlink_stmt_vdef (stmt); >release_ssa_name_fn (fun, gimple_vdef (stmt)); >gsi_remove (&i, true); > - gcc_assert (gsi_end_p (i)); > >/* We're walking here into the bbs which contain the expansion of > IFN_VA_ARG, and will not contain another IFN_VA_ARG that needs Looks good, but for some reason I hit a segfault in the linker building gengtype when I tried to bootstrap with this. I assume it's something latent and unrelated, but will need to investigate. Bill
Re: [PATCH] Fix PR79908
On Tue, Mar 14, 2017 at 10:36 PM, Bill Schmidt wrote: > >> On Mar 14, 2017, at 11:07 AM, Bill Schmidt >> wrote: >>> >>> 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); >> } >> > > Perhaps something like this? It appears to be doing better on bootstrap, and > avoids > failure on both the original problem and the new test case above: > > Index: gcc/tree-stdarg.c > === > --- gcc/tree-stdarg.c (revision 246109) > +++ gcc/tree-stdarg.c (working copy) > @@ -1058,7 +1058,13 @@ expand_ifn_va_arg_1 (function *fun) > gimplify_assign (lhs, expr, &pre); > } > else > - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); > + { > + enum gimplify_status status; > + status = gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, > + fb_lvalue | fb_mayfail); > + if (status == GS_ERROR) > + gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_rvalue); > + } > > input_location = saved_location; > pop_gimplify_context (NULL); No, I was confused in thinking gimplify_expr would handle the case properly. For just gimplifying side-effects we should use the middle-end gimplification machinery: Index: tree-stdarg.c === --- tree-stdarg.c (revision 246188) +++ tree-stdarg.c (working copy) @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3. #include "gimple-iterator.h" #include "gimple-walk.h" #include "gimplify.h" +#include "gimplify-me.h" #include "tree-into-ssa.h" #include "tree-cfg.h" #include "tree-stdarg.h" @@ -1058,12 +1059,12 @@ expand_ifn_va_arg_1 (function *fun) gimplify_assign (lhs, expr, &pre); } else - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); + force_gimple_operand (expr, &pre, false, NULL_TREE); input_location = saved_location; pop_gimplify_context (NULL); - gimple_seq_add_seq (&pre, post); + gimple_seq_add_seq_without_update (&pre, post); update_modified_stmts (pre); /* Add the sequence after IFN_VA_ARG. This splits the bb right @@ -1072,11 +1073,10 @@ expand_ifn_va_arg_1 (function *fun) gimple_find_sub_bbs (pre, &i); /* Remove the IFN_VA_ARG gimple_call. It's the last stmt in the - bb. */ + bb if we added any stmts. */ unlink_stmt_vdef (stmt); release_ssa_name_fn (fun, gimple_vdef (stmt)); gsi_remove (&i, true); - gcc_assert (gsi_end_p (i)); /* We're walking here into the bbs which contain the expansion of IFN_VA_ARG, and will not contain another IFN_VA_ARG that needs > Bill
Re: [PATCH] Fix PR79908
> On Mar 14, 2017, at 9:25 AM, Richard Biener > wrote: > > On Tue, Mar 14, 2017 at 3:20 PM, Bill Schmidt > wrote: >> >>> On Mar 14, 2017, at 7:50 AM, Bill Schmidt >>> wrote: >>> >>> On Mar 14, 2017, at 3:57 AM, Richard Biener wrote: On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt 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? So taking a step back... The original problem is that most VA_ARGs get gimplified to a single SSA var, which always passes the is_gimple_addressable check, so if the LHS has either been cast away or gone dead, we get a nice lvalue to work with and everyone is happy. For a VA_ARG of _Complex type, gimplification instead produces a COMPLEX_EXPR where a and b have been reduced to SSA vars. The is_gimple_addressable check does not pass in this case, so we can't find the mandated lvalue and ICE in the gimplifier after doing all the work. I suppose we could paper over this with: Index: gcc/tree-stdarg.c === --- gcc/tree-stdarg.c (revision 246109) +++ gcc/tree-stdarg.c (working copy) @@ -1058,7 +1058,10 @@ 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_lvalue, +fb_lvalue | fb_mayfail); ...but that is perhaps opening the gate too wide (though we could throw an error if gimplify_expr fails and expr is not a COMPLEX_EXPR). The COMPLEX_EXPR really is not addressable, but forcing any COMPLEX_EXPR to a variable in gimplify_expr also seems like overkill. (All of the tcc_binary expressions are treated the same; we would fail if somebody asked for an lvalue of a PLUS_EXPR just the same.) So offhand I don't see anything better than the above. Thoughts? Thanks, Bill
Re: [PATCH] Fix PR79908
> On Mar 14, 2017, at 11:07 AM, Bill Schmidt > wrote: >> >> 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); > } > Perhaps something like this? It appears to be doing better on bootstrap, and avoids failure on both the original problem and the new test case above: Index: gcc/tree-stdarg.c === --- gcc/tree-stdarg.c (revision 246109) +++ gcc/tree-stdarg.c (working copy) @@ -1058,7 +1058,13 @@ expand_ifn_va_arg_1 (function *fun) gimplify_assign (lhs, expr, &pre); } else - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); + { + enum gimplify_status status; + status = gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, + fb_lvalue | fb_mayfail); + if (status == GS_ERROR) + gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_rvalue); + } input_location = saved_location; pop_gimplify_context (NULL); Bill
Re: [PATCH] Fix PR79908
On Mar 14, 2017, at 9:32 AM, Bill Schmidt wrote: > >> On Mar 14, 2017, at 9:25 AM, Richard Biener >> 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); }
Re: [PATCH] Fix PR79908
> On Mar 14, 2017, at 9:25 AM, Richard Biener > wrote: > > On Tue, Mar 14, 2017 at 3:20 PM, Bill Schmidt > wrote: >> >>> On Mar 14, 2017, at 7:50 AM, Bill Schmidt >>> wrote: >>> >>> On Mar 14, 2017, at 3:57 AM, Richard Biener wrote: On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt 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
Re: [PATCH] Fix PR79908
On Tue, Mar 14, 2017 at 3:20 PM, Bill Schmidt wrote: > >> On Mar 14, 2017, at 7:50 AM, Bill Schmidt >> wrote: >> >> >>> On Mar 14, 2017, at 3:57 AM, Richard Biener >>> wrote: >>> >>> On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt >>> 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 >> >
Re: [PATCH] Fix PR79908
> On Mar 14, 2017, at 7:50 AM, Bill Schmidt wrote: > > >> On Mar 14, 2017, at 3:57 AM, Richard Biener >> wrote: >> >> On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt >> 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: 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); > > Bill >
Re: [PATCH] Fix PR79908
> On Mar 14, 2017, at 3:57 AM, Richard Biener > wrote: > > On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt > 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. Bill
Re: [PATCH] Fix PR79908
On Tue, Mar 14, 2017 at 1:04 AM, Bill Schmidt wrote: > Hi, > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79908 shows a case where > pass_stdarg ICEs attempting to gimplify a COMPLEX_EXPR with side > effects as an lvalue. The expression is not addressable, so the > gimplification fails. This patch says, hey, don't do that! > > The resulting GIMPLE looks fine afterward. Bootstrapped and tested > on powerpc64le-unknown-linux-gnu with no regressions. Is this ok > for trunk? > > Thanks, > Bill > > > [gcc] > > 2017-03-13 Bill Schmidt > > PR tree-optimization/79908 > * tree-stdarg.c (expand_ifn_va_arg_1): Don't force something to be > an lvalue that isn't addressable. > > [gcc/testsuite] > > 2017-03-13 Bill Schmidt > > PR tree-optimization/79908 > * gcc.dg/torture/pr79908.c: New file. > > > Index: gcc/testsuite/gcc.dg/torture/pr79908.c > === > --- gcc/testsuite/gcc.dg/torture/pr79908.c (nonexistent) > +++ gcc/testsuite/gcc.dg/torture/pr79908.c (working copy) > @@ -0,0 +1,12 @@ > +/* { dg-do compile } */ > + > +/* Used to fail in the stdarg pass before fix for PR79908. */ > + > +typedef __builtin_va_list __gnuc_va_list; > +typedef __gnuc_va_list va_list; > + > +void testva (int n, ...) > +{ > + va_list ap; > + _Complex int i = __builtin_va_arg (ap, _Complex int); > +} > 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); > > input_location = saved_location; >
[PATCH] Fix PR79908
Hi, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79908 shows a case where pass_stdarg ICEs attempting to gimplify a COMPLEX_EXPR with side effects as an lvalue. The expression is not addressable, so the gimplification fails. This patch says, hey, don't do that! The resulting GIMPLE looks fine afterward. Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions. Is this ok for trunk? Thanks, Bill [gcc] 2017-03-13 Bill Schmidt PR tree-optimization/79908 * tree-stdarg.c (expand_ifn_va_arg_1): Don't force something to be an lvalue that isn't addressable. [gcc/testsuite] 2017-03-13 Bill Schmidt PR tree-optimization/79908 * gcc.dg/torture/pr79908.c: New file. Index: gcc/testsuite/gcc.dg/torture/pr79908.c === --- gcc/testsuite/gcc.dg/torture/pr79908.c (nonexistent) +++ gcc/testsuite/gcc.dg/torture/pr79908.c (working copy) @@ -0,0 +1,12 @@ +/* { dg-do compile } */ + +/* Used to fail in the stdarg pass before fix for PR79908. */ + +typedef __builtin_va_list __gnuc_va_list; +typedef __gnuc_va_list va_list; + +void testva (int n, ...) +{ + va_list ap; + _Complex int i = __builtin_va_arg (ap, _Complex int); +} 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); input_location = saved_location;