[PATCH 3/6] Implement fork-based parallelism engine

[Jan Hubicka]

> 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