On Fri, Mar 17, 2017 at 6:27 PM, Bill Schmidt <wschm...@linux.vnet.ibm.com> wrote: > >> On Mar 17, 2017, at 9:52 AM, Bill Schmidt <wschm...@linux.vnet.ibm.com> >> wrote: >> >> Hi, >> >>> On Mar 17, 2017, at 6:44 AM, Richard Biener <richard.guent...@gmail.com> >>> 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: > > <L16> [1.39%]: > VA_ARG (&ap, 0B, 0B); > goto <bb 22> (<L31>); [100.00%] > > <L23> [1.39%]: > VA_ARG (&ap, 0B, 0B); > total_width_89 = total_width_17 + 337; > goto <bb 22> (<L31>); [100.00%] > > After: > > <L16> [1.39%]: > goto <bb 22> (<L31>); [100.00%] > > <L23> [1.39%]: > total_width_89 = total_width_17 + 337; > goto <bb 22> (<L31>); [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
