<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Feb 6, 2018 at 5:55 PM, Rafael Avila de Espindola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">Michael Spencer via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> writes:<br>
<br>
</span><span class="gmail-">> + .section .ponies,"ax",@progbits,unique,<wbr>1<br>
> + .globl TS<br>
> +TS:<br>
> + retq<br>
> +<br>
</span>> + .section .ponies,"ax",@progbits,unique,<wbr>2<br>
> + .globl PP<br>
> +PP:<br>
> + retq<br>
> +<br>
<br>
Delete trailing white space.<br>
<br>
> +static bool isKnownNonreorderableSection(<wbr>const OutputSection *OS) {<br>
> + return llvm::StringSwitch<bool>(OS-><wbr>Name)<br>
> + .Cases(".init", ".fini", ".init_array.", ".fini_array.", ".ctors.",<br>
> + ".dtors.", true)<br>
> + .Default(false);<br>
<br>
Why do you have a '.' in the end of the names? The output section names<br>
don't have that.<br></blockquote><div><br></div><div>Must have missed a pixel when I copied the list.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Since this should impact all sorting methods, it should probably be done<br>
in a common place. Writer<ELFT>::<wbr>sortInputSections seems a good<br>
candidate. It can probably become a single loop over the output sections<br>
that "switches" on the name and call the appropriate sort function.<br>
<br>
Alternatively, OutputSection::sort could be the one that checks the<br>
name and we would have just<br>
<span class="gmail-"><br>
template <class ELFT> void Writer<ELFT>::<wbr>sortInputSections() {<br>
</span> // builds an empty dense map if we don't have --symbol-ordering-file or call-graph-ordering-file<br>
<span class="gmail-"> DenseMap<SectionBase *, int> Order = buildSectionOrder();<br>
<br>
</span> for (BaseCommand *Base : Script->SectionCommands)<br>
if (auto *Sec = dyn_cast<OutputSection>(Base))<br>
if (Sec->Live)<br>
Sec->sort([&](InputSectionBase *S) { return Order.lookup(S); });<br>
}<br>
<br>
And sortInitFini and sortCtorsDtors are just helpers called by sort.<br>
<br>
Please send a patch fixing just this issue with --symbol-ordering-file<br>
so that it can be reviewed independently.<br></blockquote><div><br></div><div><a href="https://reviews.llvm.org/D43038">https://reviews.llvm.org/D43038</a><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-"><br>
> +}<br>
> +<br>
> +// Take the edge list in Config->CallGraphProfile, resolve symbol names to<br>
> +// Symbols, and generate a graph between InputSections with the provided<br>
> +// weights.<br>
> +CallGraphSort::CallGraphSort(<wbr>) {<br>
</span>> + MapVector<std::pair<const InputSectionBase *, const InputSectionBase *>,<br>
<span class="gmail-">> + uint64_t> &Profile = Config->CallGraphProfile;<br>
</span>> + DenseMap<const InputSectionBase *, NodeIndex> SecToNode;<br>
<span class="gmail-">> + DenseMap<Edge, EdgeIndex, EdgeDenseMapInfo> EdgeMap;<br>
> +<br>
> + auto GetOrCreateNode = [&](const InputSectionBase *IS) -> NodeIndex {<br>
> + auto Res = SecToNode.insert(std::make_<wbr>pair(IS, Nodes.size()));<br>
> + if (Res.second)<br>
> + Nodes.emplace_back(IS);<br>
> + return Res.first->second;<br>
> + };<br>
> +<br>
> + // Create the graph.<br>
> + for (const auto &C : Profile) {<br>
> + const InputSectionBase *FromSB = C.first.first;<br>
> + const InputSectionBase *ToSB = C.first.second;<br>
> + uint64_t Weight = C.second;<br>
> +<br>
> + if (Weight == 0)<br>
> + continue;<br>
> +<br>
</span>> + if (FromSB->getOutputSection() != ToSB->getOutputSection())<br>
> + continue;<br>
<br>
We can probably filter these cases before putting them in<br>
Config->CallGraphProfile.<br></blockquote><div><br></div><div>That would then need to be duplicated for when the call graph comes from the object file.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Please add a comment on why we should ignore and edge if the output<br>
sections are different. I can see that we don't control the layout of<br>
output sections, but it is not clear that one order is inherently better<br>
than the other.<br>
<br>
Should we ignore an edge if the sections are in different ISDs?<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div><div class="gmail_extra">- Michael Spencer</div></div>