[llvm] r322595 - Specify inline for isWhitespace in CommandLine.cpp

[Shoaib Meenai via llvm-commits]

Specifically, it looks like on Windows platforms, CMake's default CMAKE_<LANG>_FLAGS_RELWITHDEBINFO include /Ob1, which prevents inline expansion of functions not explicitly marked inline (https://docs.microsoft.com/en-us/cpp/build/reference/ob-inline-function-expansion).

From: llvm-commits <llvm-commits-bounces at lists.llvm.org> on behalf of Rui Ueyama via llvm-commits <llvm-commits at lists.llvm.org>
Reply-To: Rui Ueyama <ruiu at google.com>
Date: Monday, January 22, 2018 at 3:26 PM
To: Chandler Carruth <chandlerc at gmail.com>
Cc: llvm-commits <llvm-commits at lists.llvm.org>
Subject: Re: [llvm] r322595 - Specify inline for isWhitespace in CommandLine.cpp

This change was made by a wrong assumption that -DCMAKE_BUILD_TYPE=RelWithDebugInfo would produce the same executable as -DCMAKE_BUILD_TYPE=Release modulo debug info. Turned out that's not true -- it at least disables some optimizations such as function inlining. This change doesn't do any harm, but I'll revert it to avoid confusion.

On Mon, Jan 22, 2018 at 1:45 PM, Chandler Carruth <chandlerc at gmail.com<mailto:chandlerc at gmail.com>> wrote:
Yeah, this really shouldn't be necessary and seems really questionable... I cannot imagine a way that LLVM requires this, and if it LLVM doesn't require this I wonder if we should just move to benchmarking a self-hosted LLD rather than worrying about the very detailed performance characteristics with MSCV-built LLD. This seems like a pretty glaring deficiency in the MSVC inliner that we're papering over.

On Mon, Jan 22, 2018 at 9:57 AM David Blaikie <dblaikie at gmail.com<mailto:dblaikie at gmail.com>> wrote:
Is this with a selfhost build? (ie: is clang failing to inline this function where it should) - should we look into why the compiler is failing to catch this case?

On Tue, Jan 16, 2018 at 12:53 PM Rui Ueyama via llvm-commits <llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>> wrote:
Author: ruiu
Date: Tue Jan 16 12:52:32 2018
New Revision: 322595

URL: http://llvm.org/viewvc/llvm-project?rev=322595&view=rev<https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D322595-26view-3Drev&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=JRfkF7cP7jwX_p0nshQi_wg4pHcfFlViNbqR7ZK1ogM&s=Vm9HaBlLA9C9zTVpHW2KpFS1nm3lxXfRuaJpy_wb5AA&e=>
Log:
Specify inline for isWhitespace in CommandLine.cpp

Patch by Takuto Ikuta.

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

Modified:
    llvm/trunk/lib/Support/CommandLine.cpp

Modified: llvm/trunk/lib/Support/CommandLine.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/CommandLine.cpp?rev=322595&r1=322594&r2=322595&view=diff<https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_Support_CommandLine.cpp-3Frev-3D322595-26r1-3D322594-26r2-3D322595-26view-3Ddiff&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=JRfkF7cP7jwX_p0nshQi_wg4pHcfFlViNbqR7ZK1ogM&s=c_5_O0wtNPhV0wUsZneTQz5qMJOScKgSTfCJrfSsdyY&e=>
==============================================================================
--- llvm/trunk/lib/Support/CommandLine.cpp (original)
+++ llvm/trunk/lib/Support/CommandLine.cpp Tue Jan 16 12:52:32 2018
@@ -688,7 +688,7 @@ static bool EatsUnboundedNumberOfValues(
          O->getNumOccurrencesFlag() == cl::OneOrMore;
 }

-static bool isWhitespace(char C) {
+static inline bool isWhitespace(char C) {
   return C == ' ' || C == '\t' || C == '\r' || C == '\n';
 }



_______________________________________________
llvm-commits mailing list
llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits<https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Dcommits&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=JRfkF7cP7jwX_p0nshQi_wg4pHcfFlViNbqR7ZK1ogM&s=PmT80Pe7_nbc-iU6QthOcKi2rAAu7Iq61aaC25iVSNg&e=>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180122/34ca0632/attachment.html>