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

[Michael Spencer via llvm-commits]

On Thu, Mar 8, 2018 at 10:17 AM, Rafael Avila de Espindola <
rafael.espindola at gmail.com> wrote:

>
> > +static void readCallGraph(MemoryBufferRef MB) {
> > +  // Build a map from symbol name to section
> > +  DenseMap<StringRef, InputSectionBase *> SymbolSection;
> > +  for (InputFile *File : ObjectFiles)
> > +    for (Symbol *Sym : File->getSymbols())
> > +      if (auto *D = dyn_cast<Defined>(Sym))
> > +        if (auto *IS = dyn_cast_or_null<InputSectionBase>(D->Section))
> > +          SymbolSection[D->getName()] = IS;
> > +
> > +  std::vector<StringRef> Lines = args::getLines(MB);
> > +  for (StringRef L : Lines) {
> > +    SmallVector<StringRef, 3> Fields;
> > +    L.split(Fields, ' ');
> > +    if (Fields.size() != 3)
> > +      fatal("parse error");
> > +    uint64_t Count;
> > +    if (!to_integer(Fields[2], Count))
> > +      fatal("parse error");
> > +    InputSectionBase *FromSec = SymbolSection.lookup(Fields[0]);
> > +    InputSectionBase *ToSec = SymbolSection.lookup(Fields[1]);
> > +    if (FromSec && ToSec)
> > +      Config->CallGraphProfile[std::make_pair(FromSec, ToSec)] +=
> Count;
>
> Should this be using FromSec->Repl, ToSec->Repl to handle ICF? If so
> please update the code and add a new test (the one in the patch is
> getting big).
>

I'll look into this.


>
> > +  }
> > +}
>
> With --symbol-ordering-file we produce warnings if, for example, the
> symbol is found to be absolute. The same issue applies here, no? Can you
> refactor the code so that we print the same warnings for
> --call-graph-ordering-file?
>

Given how the call graph files are likely to be produced I would expect
warning on that would just be noise.


>
> > +using ClusterIndex = std::ptrdiff_t;
> > +using SectionIndex = std::ptrdiff_t;
> > +using EdgeIndex = std::ptrdiff_t;
>
> Does this means that we use 64 bit integers for indexes? Why?
>

On a 64 bit platform, yes.  It's used because it can index all sections on
whatever platform it's on.  It's signed because the only reason to use
unsigned types is if you need the range, are bit twiddling, or need modular
arithmetic.


>
> > +// Used for calculating an comparing density.  Use soft-float for
> determinism.
> > +struct Double : APFloat {
>
> This used to be a problem with x87, but do we support a system today
> where this is a problem?
>
> Cheers,
> Rafael
>

We don't use float/double anywhere else in the compiler.  I do not have
high confidence that all supported versions of clang, gcc, and msvc for all
supported targets will have bit identical behavior here.

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