[PATCH] D36351: [lld][ELF] Add profile guided section layout

[Michael Spencer via llvm-commits]

On Tue, Feb 6, 2018 at 5:55 PM, Rafael Avila de Espindola <
rafael.espindola at gmail.com> wrote:

> Michael Spencer via Phabricator <reviews at reviews.llvm.org> writes:
>
> > +    .section    .ponies,"ax", at progbits,unique,1
> > +    .globl TS
> > +TS:
> > +    retq
> > +
> > +    .section    .ponies,"ax", at progbits,unique,2
> > +    .globl PP
> > +PP:
> > +    retq
> > +
>
> Delete trailing white space.
>
> > +static bool isKnownNonreorderableSection(const OutputSection *OS) {
> > +  return llvm::StringSwitch<bool>(OS->Name)
> > +      .Cases(".init", ".fini", ".init_array.", ".fini_array.",
> ".ctors.",
> > +             ".dtors.", true)
> > +      .Default(false);
>
> Why do you have a '.' in the end of the names? The output section names
> don't have that.
>

Must have missed a pixel when I copied the list.


>
> Since this should impact all sorting methods, it should probably be done
> in a common place. Writer<ELFT>::sortInputSections seems a good
> candidate. It can probably become a single loop over the output sections
> that "switches" on the name and call the appropriate sort function.
>
> Alternatively, OutputSection::sort could be the one that checks the
> name and we would have just
>
> template <class ELFT> void Writer<ELFT>::sortInputSections() {
>   // builds an empty dense map if we don't have --symbol-ordering-file or
> call-graph-ordering-file
>   DenseMap<SectionBase *, int> Order = buildSectionOrder();
>
>   for (BaseCommand *Base : Script->SectionCommands)
>     if (auto *Sec = dyn_cast<OutputSection>(Base))
>       if (Sec->Live)
>           Sec->sort([&](InputSectionBase *S) { return Order.lookup(S); });
> }
>
> And sortInitFini and sortCtorsDtors are just helpers called by sort.
>
> Please send a patch fixing just this issue with --symbol-ordering-file
> so that it can be reviewed independently.
>

https://reviews.llvm.org/D43038


>
> > +}
> > +
> > +// Take the edge list in Config->CallGraphProfile, resolve symbol names
> to
> > +// Symbols, and generate a graph between InputSections with the provided
> > +// weights.
> > +CallGraphSort::CallGraphSort() {
> > +  MapVector<std::pair<const InputSectionBase *, const InputSectionBase
> *>,
> > +            uint64_t> &Profile = Config->CallGraphProfile;
> > +  DenseMap<const InputSectionBase *, NodeIndex> SecToNode;
> > +  DenseMap<Edge, EdgeIndex, EdgeDenseMapInfo> EdgeMap;
> > +
> > +  auto GetOrCreateNode = [&](const InputSectionBase *IS) -> NodeIndex {
> > +    auto Res = SecToNode.insert(std::make_pair(IS, Nodes.size()));
> > +    if (Res.second)
> > +      Nodes.emplace_back(IS);
> > +    return Res.first->second;
> > +  };
> > +
> > +  // Create the graph.
> > +  for (const auto &C : Profile) {
> > +    const InputSectionBase *FromSB = C.first.first;
> > +    const InputSectionBase *ToSB = C.first.second;
> > +    uint64_t Weight = C.second;
> > +
> > +    if (Weight == 0)
> > +      continue;
> > +
> > +    if (FromSB->getOutputSection() != ToSB->getOutputSection())
> > +      continue;
>
> We can probably filter these cases before putting them in
> Config->CallGraphProfile.
>

That would then need to be duplicated for when the call graph comes from
the object file.


>
> Please add a comment on why we should ignore and edge if the output
> sections are different. I can see that we don't control the layout of
> output sections, but it is not clear that one order is inherently better
> than the other.
>
> Should we ignore an edge if the sections are in different ISDs?
>

Depends on if you want the linker script to always override symbol
ordering. Given the way symbol ordering is generally generated and what
ISDs are used for, we probably shouldn't sort between ISDs.


>
> Cheers,
> Rafael
>

- Michael Spencer
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180207/dcac3d6a/attachment.html>