<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On May 22, 2011, at 6:50 AM, Matthieu Monrocq wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><br><br><div class="gmail_quote">2011/5/20 Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:kyrtzidis@apple.com">kyrtzidis@apple.com</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div style="word-wrap:break-word"><div>Applying clang_stringrefize_diagnostics.diff causes DiagnosticIDs.cpp to take > 10 mins to compile with optimizations on. See <a href="http://llvm.org/PR9956" target="_blank">http://llvm.org/PR9956</a></div>
<div><br></div><div>-Argyrios</div><div><br></div><div><div><div><div></div><div style="word-wrap:break-word"><div>On May 19, 2011, at 12:46 PM, Argyrios Kyrtzidis wrote:<blockquote type="cite"><div><font>Index: tools/libclang/CIndexDiagnostic.cpp</font></div>
<div><font>===================================================================</font></div><div><font>--- tools/libclang/CIndexDiagnostic.cpp</font><span style="white-space:pre-wrap"><font> </font></span><font>(révision 131339)</font></div>
<div><font>+++ tools/libclang/CIndexDiagnostic.cpp</font><span style="white-space:pre-wrap"><font> </font></span><font>(copie de travail)</font></div><div><font>@@ -220,7 +220,8 @@</font></div><div><font> return createCXString("");</font></div>
<div><font> </font></div><div><font> unsigned ID = StoredDiag->Diag.getID();</font></div><div><font>- if (const char *Option = DiagnosticIDs::getWarningOptionForDiag(ID)) {</font></div><div><font>+ llvm::StringRef Option = DiagnosticIDs::getWarningOptionForDiag(ID);</font></div>
<div><font>+ if (Option.data()) {</font></div><div><font> if (Disable)</font></div></blockquote></div><div><div><br></div></div><div>Isn't it a bit better and more descriptive if you used !.empty() instead of .data() ? This applies to the rest of the diffs.</div>
</div></div></div></div></div></blockquote><div><br>Changed, it is indeed better, especially since could end up with a valid pointer but a 0 length.<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div style="word-wrap: break-word;"><div><div><div><div style="word-wrap: break-word;"><div><br></div><div><blockquote type="cite"><div><font> // -Wsystem-headers is a special case, not driven by the option table. It</font></div>
<div><font> // cannot be controlled with -Werror.</font></div><div><font>- if (OptEnd-OptStart == 14 && memcmp(OptStart, "system-headers", 14) == 0) {</font></div><div><font>+ if (Opt == "system-headers") {</font></div>
<div><font> Diags.setSuppressSystemWarnings(!isPositive);</font></div></blockquote></div><div><div><br></div></div><div>Probably a not-worth-much micro-optimization but with the changes in lib/Frontend/Warnings.cpp you are introducing multiple strlen calls inside the loop, maybe use something like llvm::StringRef("system-headers", 14) or initialize the StringRefs outside the loop ?</div>
<div><br></div></div></div></div></div></div></blockquote><div> </div><div>As you noticed in a private answer, since the constructor of StringRef is inlined within the header, the call to strlen should end up being removed by the compiler and turned into a constant. Leaving the C-strings as they are makes for much more readable code. <br>
</div><div> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;"><div><div><div><div style="word-wrap: break-word;">
<div></div><div>Thanks for your work!</div><div><br></div><div>-Argyrios</div><br></div></div></div></div></div></blockquote><div><br>The initial patch for StringRef-ization had been done without changing the code generated by TableGen. It introduced about ~10k llvm::StringRef("xx") statements, which I believe were the root cause of the compilation time going overboard.<br>
<br>I have reviewed my patch by completely changing the files generated by TableGen for the diagnostics, and notably generating all StringRef constructions either as llvm::StringRef() for empty strings or llvm::StringRef("xx", 2) for real strings (with the necessary escaping).<br>
<br>This seems to have solved the performance issue:<br><br>[ 98%] Building CXX object tools/clang/lib/Basic/CMakeFiles/clangBasic.dir/DiagnosticIDs.cpp.obj<br>Linking CXX static library ../../../../lib/libclangBasic.a<br>
[100%] Built target clangBasic<br><br>real 2m37.453s<br>user 0m33.109s<br>sys 0m36.482s<br><br>(that is for the whole of libclangBasic.a and its dependencies, with -j 1, and only DiagnosticIDs.cpp being changed)<br>
<br><br>Now, instead of generating a macro call with some arguments, I directly generate what's required for the table definitions, which is a consequent change.<br><br>It means that both patches (to llvm and clang) shall be applied / fallen back together.<br>
<br><br>Tested in both Debug and Release, this time :)<br></div></div></blockquote><div><br></div><div>Unfortunately even this iteration takes forever to compile DiagnosticIDs.cpp in release mode.</div><div>The underlying problem is that adding StringRefs to the diagnostic data structures results in a huge</div><div>global-var-init function that is needed to do the StringRef constructions, in contrast to the statically constructed arrays that we have now.</div><div><br></div><div>I took the liberty and used a different approach; the data structures now include the size of the strings and we get a StringRef using member functions</div><div>on the structures.</div><div><br></div><div>Committed your patch in r132047; my tweaks are in DiagnosticIDs.cpp and I just made a couple of changes to ClangDiagnosticsEmitter.cpp (llvm r132046).</div><div><br></div><div>Thanks!</div><br><blockquote type="cite"><div class="gmail_quote"><div><br>-- Matthieu<br></div></div><br>
<span><llvm_stringrefize_diagnostics.diff></span><span><clang_stringrefize_diagnostics.diff></span>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br></blockquote></div><br></body></html>