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

Mon Jan 22 17:47:16 PST 2018

On Mon, Jan 22, 2018 at 3:41 PM Shoaib Meenai via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

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

I'm surprised that also applies to *static* functions... Might be worth
filing a bug suggesting that it not apply to static functions.... but yeah,
dunno, mabye it should and we should just be careful what flags we pass in
these configs.

Thanks to both of you for tracking this down, fascinating!

>
>
> *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>
> 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> 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> 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
> 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=>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180123/be0d8cc4/attachment.html>