[cfe-commits] [cfe-dev] How to propose patches ?

[Matthieu Monrocq]

2011/5/20 Argyrios Kyrtzidis <kyrtzidis at apple.com>

> 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 :)

-- Matthieu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110522/7244a847/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm_stringrefize_diagnostics.diff
Type: application/octet-stream
Size: 4165 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110522/7244a847/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang_stringrefize_diagnostics.diff
Type: application/octet-stream
Size: 26350 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110522/7244a847/attachment-0001.obj>