<div dir="ltr">Alrighty. All comments addressed. I'll send a message to the mailing list about llvm::Record::isSubClassOf(StringRef Name).</div><div class="gmail_extra"><br clear="all"><div><div class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div dir="ltr">Sincerely,<br>Alexander Riccio<br>--<br>"Change the world or go home."<div><a href="http://about.me/ariccio" target="_blank">about.me/ariccio</a></div><div><a href="http://about.me/ariccio" target="_blank"><br></a></div><div>If left to my own devices, I will build more.</div><div>⁂<br></div></div></div></div></div></div></div>
<br><div class="gmail_quote">On Wed, Feb 3, 2016 at 12:15 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Tue, Feb 2, 2016 at 5:35 PM, Alexander Riccio via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br></span><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">ariccio created this revision.<br>
ariccio added reviewers: rnk, qcolombet, craig.topper.<br>
ariccio added a subscriber: llvm-commits.<br>
<br>
This patch adds a reserve call to an expensive function (`llvm::LoadIntrinsics`), and may fix a few other low hanging performance fruit (I've put them in comments for now, so we can discuss).<br>
<br>
**Motivation:**<br>
<br>
As I'm sure other developers do, when I build LLVM, I build the entire project with the same config (`Debug`, `MinSizeRel`, `Release`, or `RelWithDebInfo`). However, the `Debug` config also builds llvm-tblgen in `Debug` mode. Later build steps that run llvm-tblgen then can actually be the slowest steps in the entire build. Nobody likes slow builds.<br>
<br>
<br>
<br>
<a href="http://reviews.llvm.org/D16832" rel="noreferrer" target="_blank">http://reviews.llvm.org/D16832</a><br>
<br>
Files:<br>
C:/LLVM/llvm/include/llvm/TableGen/Record.h<br>
C:/LLVM/llvm/utils/TableGen/CodeGenInstruction.cpp<br>
C:/LLVM/llvm/utils/TableGen/CodeGenTarget.cpp<br>
<br>
Index: C:/LLVM/llvm/utils/TableGen/CodeGenInstruction.cpp<br>
===================================================================<br>
--- C:/LLVM/llvm/utils/TableGen/CodeGenInstruction.cpp<br>
+++ C:/LLVM/llvm/utils/TableGen/CodeGenInstruction.cpp<br>
@@ -49,6 +49,11 @@<br>
<br>
unsigned MIOperandNo = 0;<br>
std::set<std::string> OperandNames;<br>
+<br>
+ // TODO: can we reserve space for e elements in OperandList?<br></blockquote><div><br></div></span><div>Seems reasonable to me - looks like we know exactly how many elements we're going to add (there's not some (or any) complex selection criteria in the loop that means we add fewer items than the number we iterate over.<br><br>Please just checkin the reserve call, don't bother adding the TODO.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ // Profiling a debug build with ETW revealed that the emplace_back<br>
+ // at the very bottom of this loop can take up to 227ms, largely<br>
+ // spent in reallocation.<br>
for (unsigned i = 0, e = InDI->getNumArgs()+OutDI->getNumArgs(); i != e; ++i){<br>
Init *ArgInit;<br>
std::string ArgName;<br>
Index: C:/LLVM/llvm/include/llvm/TableGen/Record.h<br>
===================================================================<br>
--- C:/LLVM/llvm/include/llvm/TableGen/Record.h<br>
+++ C:/LLVM/llvm/include/llvm/TableGen/Record.h<br>
@@ -1307,9 +1307,12 @@<br>
}<br>
<br>
bool isSubClassOf(StringRef Name) const {<br>
- for (const auto &SCPair : SuperClasses)<br>
+ for (const auto &SCPair : SuperClasses) {<br>
+ // TODO: getNameInitAsString copy constructs a new std::string,<br>
+ // yet we're only using it in this comparison. Is there a better way?<br></blockquote><div><br></div></span><div>Skip this TODO - no, it doesn't look immediately like we can do much. I'd probably discuss it more on-list, rather than adding a TODO just yet.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
if (SCPair.first->getNameInitAsString() == Name)<br>
return true;<br>
+ }<br>
return false;<br>
}<br>
<br>
Index: C:/LLVM/llvm/utils/TableGen/CodeGenTarget.cpp<br>
===================================================================<br>
--- C:/LLVM/llvm/utils/TableGen/CodeGenTarget.cpp<br>
+++ C:/LLVM/llvm/utils/TableGen/CodeGenTarget.cpp<br>
@@ -441,12 +441,24 @@<br>
std::vector<Record*> I = RC.getAllDerivedDefinitions("Intrinsic");<br>
<br>
std::vector<CodeGenIntrinsic> Result;<br>
+<br>
+ // Allocate enough memory to hold enough for worst case scenario<br>
+ // in the following loop. Profiling a debug build with ETW<br>
+ // revealed that push_back in the loop could take up to 280ms,<br>
+ // largely spent in reallocation.<br></blockquote><div><br></div></span><div>Don't bother adding the comment - it seems pretty obvious why this reserve call is here.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ Result.reserve(I.size());<br>
<br>
for (unsigned i = 0, e = I.size(); i != e; ++i) {<br>
bool isTarget = I[i]->getValueAsBit("isTarget");<br>
if (isTarget == TargetOnly)<br>
Result.push_back(CodeGenIntrinsic(I[i]));<br>
}<br>
+<br>
+ // TODO: does this lambda need to accept parameters by value?<br>
+ // Profiling a debug build with ETW revealed that each call of<br>
+ // std::_Unguarded_partition (an internal function, called by std::sort),<br>
+ // can spend hundreds of milliseconds inside this lambda calling<br>
+ // ~CodeGenIntrinsic(), which is quite wasteful.<br></blockquote><div><br></div></span><div>Just make the parameters const ref and don't bother adding a TODO/comment here.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
std::sort(Result.begin(), Result.end(),<br>
[](CodeGenIntrinsic LHS, CodeGenIntrinsic RHS) {<br>
return LHS.Name < RHS.Name;<br>
<br>
<br>
<br></span><span class="">_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></span></blockquote></div><br></div></div>
</blockquote></div><br></div>