Specify inline for isWhitespace in CommandLine.cpp

[takuto.ikuta]

Summary

In chromium's component build, there are many directive sections and commandline parsing takes much time.
This patch is for speed up of lld in RelWithDebInfo build by forcing inline heavily called isWhitespace function.

10 times link perf stats of blink_core.dll changed like below.

master:
TotalSeconds: 9.8764878
TotalSeconds: 10.1455242
TotalSeconds: 10.075279
TotalSeconds: 10.3397347
TotalSeconds: 9.8361665
TotalSeconds: 9.9544441
TotalSeconds: 9.8960686
TotalSeconds: 9.8877865
TotalSeconds: 10.0551879
TotalSeconds: 10.0492254
Avg: 10.01159047

with this patch:
TotalSeconds: 8.8696762
TotalSeconds: 9.1021585
TotalSeconds: 9.0233893
TotalSeconds: 9.1886175
TotalSeconds: 9.156954
TotalSeconds: 9.0978564
TotalSeconds: 9.1316824
TotalSeconds: 8.8354606
TotalSeconds: 9.2549431
TotalSeconds: 9.4473085
Avg: 9.11080465

[ruiu]

LGTM but what compiler are you using? I thought that this static function would be inlined even if you don't explicitly add "inline".

[takuto.ikuta]

Thank you for review.

I use cl.exe in vs2017.
cmake gives /Ob1 flags in RelWithDebInfo, that prevents the function being inlined.
https://msdn.microsoft.com/en-us/library/47238hez.aspx

[ruiu]

I tried that and you are right. If you pass LLVM_BUILD_TYPE=RelWithDebInfo, it passes /Ob1 which disables function inlining except for functions that explicitly marked "inline". That logic is not in our configuration. CMake passes /Ob1.

Looks like we shouldn't use RelWithDebInfo at all when doing profiling. Instead we should always pass LLVM_BUILD_TYPE=Release with CMAKE_C{,XX}_FLAGS=/Zi.

[takuto.ikuta]

I see, I'll use such options next time. Thanks.

[zturner]

In D42094#977730, @ruiu wrote:

I tried that and you are right. If you pass LLVM_BUILD_TYPE=RelWithDebInfo, it passes /Ob1 which disables function inlining except for functions that explicitly marked "inline". That logic is not in our configuration. CMake passes /Ob1.

Looks like we shouldn't use RelWithDebInfo at all when doing profiling. Instead we should always pass LLVM_BUILD_TYPE=Release with CMAKE_C{,XX}_FLAGS=/Zi.

Note that this doesn't do what it seems like it should intuitively do. CMake has its own set of ideas about what to initialize CMAKE_CXX_CFLAGS and CMAKE_C_FLAGS to. So if you write cmake -DCMAKE_CXX_FLAGS=/Zi then you're erasing all the default options CMake adds for you. That means you won't even get any optimization anymore.

it's not clear what the best way to handle this is. Setting environment variables is never a good option, but it works. What I'm currently doing is running cmake as normal with -DCMAKE_BUILD_TYPE=Release, then editing my CMakeCache.txt and editing the line for CMAKE_CXX_FLAGS_RELEASE and appending /Zi to it. This ensures that you get all the flags that it was going to put there for you originally *plus* /Zi.

Another option would be to add a new configuration RelWithProfilingInfo which does the right thing.

[takuto.ikuta]

Thanks, I'll do that next time.
RelWithProfilingInfo looks nice.

llvm/lib/Support/CommandLine.cpp

return O->getNumOccurrencesFlag() == cl::Required ||
          O->getNumOccurrencesFlag() == cl::OneOrMore;
 }
 
 static bool EatsUnboundedNumberOfValues(const Option *O) {
   return O->getNumOccurrencesFlag() == cl::ZeroOrMore ||
          O->getNumOccurrencesFlag() == cl::OneOrMore;
 }
 
-static bool isWhitespace(char C) {
+static inline bool isWhitespace(char C) {
   return C == ' ' || C == '\t' || C == '\r' || C == '\n';
 }
 
 static bool isQuote(char C) { return C == '\"' || C == '\''; }
 
 void cl::TokenizeGNUCommandLine(StringRef Src, StringSaver &Saver,
                                 SmallVectorImpl<const char *> &NewArgv,
                                 bool MarkEOLs) {