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

[Rafael Avila de Espindola via llvm-commits]

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.

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.

> +}
> +
> +// 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.

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?

Cheers,
Rafael