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

[Argyrios Kyrtzidis]

On May 22, 2011, at 6:50 AM, Matthieu Monrocq wrote:

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

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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110524/04e993f8/attachment.html>