> Hi, Honza. > > Thank you for your detailed review! > > On 08/27, Jan Hubicka wrote: > > > diff --git a/gcc/cgraph.c b/gcc/cgraph.c > > > index c0b45795059..22405098dc5 100644 > > > --- a/gcc/cgraph.c > > > +++ b/gcc/cgraph.c > > > @@ -226,6 +226,22 @@ cgraph_node::delete_function_version_by_decl (tree > > > decl) > > > decl_node->remove (); > > > } > > > > > > +/* Release function dominator info if present. */ > > > + > > > +void > > > +cgraph_node::maybe_release_dominators (void) > > > +{ > > > + struct function *fun = DECL_STRUCT_FUNCTION (decl); > > > + > > > + if (fun && fun->cfg) > > > + { > > > + if (dom_info_available_p (fun, CDI_DOMINATORS)) > > > + free_dominance_info (fun, CDI_DOMINATORS); > > > + if (dom_info_available_p (fun, CDI_POST_DOMINATORS)) > > > + free_dominance_info (fun, CDI_POST_DOMINATORS); > > > + } > > > +} > > > > I am not sure if that needs to be member function, but if so we want to > > merge it with other places in cgraph.c and cgraphunit.c where dominators > > are freed. I do not think you need to check avalability. > > This is necessary to remove some nodes from the callgraph. For some > reason, if I node->remove () and it still have the dominance info > available, it will fail some assertions on the compiler. > > However, with regard to code layout, this can be moved to lto-cgraph.c, > as it is only used there.
node->remove () is supposed to work here. I relalize that we remove random nodes currently only via the unreachable code removal that also does node->remove_body () so there may be some bug. > > > + /* Get symtab node by order. */ > > > + static symtab_node *find_by_order (int order); > > > > This is quadratic and moreover seems unused. Why do you add it? > > I added this for debugging, since I used this a lot inside GDB. > Sure, I can remove this without any problems, or print a warning > for the developer to avoid calling this in production code. We could keep this as separate patch, since it is independent of rest of changes. Having more debug_* functions is fine with me, but I would like to having nondebug members since sooner or later someone will use them in non-debug code. > > > + /* FIXME: Find out why body-removed nodes are marked for output. */ > > > + if (body_removed) > > > + return; > > > > Indeed, we should know :) > > Looks like this was due an early problem. I removed this and bootstrap > is working OK. Cool, random changes like this looks suspicious :) > > > > This looks odd, we have other places where we parse number from command > > line :) > > isdigit () is poisoned in GCC. But I guess I should look how -flto= > does this. Seems we simply do atoi and compare with 0 that is invalid. > > > > Messing with WPA/ltrans flags is not good idea. You already have > > split_output for parallel build. I sort of see why you set ltrans, but > > why WPA? > > Some assertions expected flag_wpa to be present. Sure, I can add > split_outputs in these assertions. Yep, that would be cleaner. > > > > There is boundary computation in lto-cgraph.c so we should merge the > > logic... > > Agree. > > > If you keep the lto-partition datastructures it will compute boundary > > for you and you can just remove the rest (I got that working at one > > point). > > This is interesting, because I could not get that working out of the > box. The lto_promote_cross_statics did not provide a fully working > boundary that I could simple remove everything else. If you take a > closer look, you will see that I am using the already computed boundary > as a base, and incrementing that with the extra stuff required > (mainly inline clones body). What problems exactly you run into when you remove the extra code? > > > > Aha, that is what it does :) > > I wonder if creating the file conditionally (and late) can not lead to > > race condition where we will use same tmp file name for other build > > executed in parallel by make. > > Hummm. True. Added to my TODO list :) > Well, I never had any sort of issues with race condition here, even > after stressing it, but this certainly is not proof that it is free of > race conditition :) I really do not know how this works, since we create other files with delay too. I suppose we get a tmp filename that is tested to be unique and then we simply assume that we can derive names from it. Honza