On May 22, 2011, at 6:50 AM, Matthieu Monrocq wrote: > > > 2011/5/20 Argyrios Kyrtzidis <[email protected]> > Applying clang_stringrefize_diagnostics.diff causes DiagnosticIDs.cpp to take > > 10 mins to compile with optimizations on. See http://llvm.org/PR9956 > > -Argyrios > > On May 19, 2011, at 12:46 PM, Argyrios Kyrtzidis wrote: >> >> Index: tools/libclang/CIndexDiagnostic.cpp >> =================================================================== >> --- tools/libclang/CIndexDiagnostic.cpp (révision 131339) >> +++ tools/libclang/CIndexDiagnostic.cpp (copie de travail) >> @@ -220,7 +220,8 @@ >> return createCXString(""); >> >> unsigned ID = StoredDiag->Diag.getID(); >> - if (const char *Option = DiagnosticIDs::getWarningOptionForDiag(ID)) { >> + llvm::StringRef Option = DiagnosticIDs::getWarningOptionForDiag(ID); >> + if (Option.data()) { >> if (Disable) > > > Isn't it a bit better and more descriptive if you used !.empty() instead of > .data() ? This applies to the rest of the diffs. > > Changed, it is indeed better, especially since could end up with a valid > pointer but a 0 length. > > >> // -Wsystem-headers is a special case, not driven by the option table. >> It >> // cannot be controlled with -Werror. >> - if (OptEnd-OptStart == 14 && memcmp(OptStart, "system-headers", 14) == >> 0) { >> + if (Opt == "system-headers") { >> Diags.setSuppressSystemWarnings(!isPositive); > > > 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 ? > > > 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. > > Thanks for your work! > > -Argyrios > > > 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. > > 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). > > This seems to have solved the performance issue: > > [ 98%] Building CXX object > tools/clang/lib/Basic/CMakeFiles/clangBasic.dir/DiagnosticIDs.cpp.obj > Linking CXX static library ../../../../lib/libclangBasic.a > [100%] Built target clangBasic > > real 2m37.453s > user 0m33.109s > sys 0m36.482s > > (that is for the whole of libclangBasic.a and its dependencies, with -j 1, > and only DiagnosticIDs.cpp being changed) > > > 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. > > It means that both patches (to llvm and clang) shall be applied / fallen back > together. > > > Tested in both Debug and Release, this time :)
Unfortunately even this iteration takes forever to compile DiagnosticIDs.cpp in release mode. The underlying problem is that adding StringRefs to the diagnostic data structures results in a huge global-var-init function that is needed to do the StringRef constructions, in contrast to the statically constructed arrays that we have now. 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 on the structures. Committed your patch in r132047; my tweaks are in DiagnosticIDs.cpp and I just made a couple of changes to ClangDiagnosticsEmitter.cpp (llvm r132046). Thanks! > > -- Matthieu > > <llvm_stringrefize_diagnostics.diff><clang_stringrefize_diagnostics.diff>_______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
