[llvm] ff837aa - Actually, don't try to use __builtin_strlen in StringRef.h before VS 2019

Fri Feb 7 04:04:56 PST 2020

Thanks! I've cherry-picked the change to 10.x in
ed368ba5a963cb988c7466cb634ec1b56807138f

On Fri, Feb 7, 2020 at 1:27 AM Yung, Douglas <douglas.yung at sony.com> wrote:
>
> Hi Hans,
>
> Sorry for the delay, just wanted to do some testing. I can confirm that it does now build quickly for us. The build times for CheckerRegistry.cpp went from 7-8 minutes to 20-30 seconds after your change on my machine.
>
> Douglas Yung
>
> -----Original Message-----
> From: Hans Wennborg <hans at chromium.org>
> Sent: Thursday, February 6, 2020 3:35
> To: Yung, Douglas <douglas.yung at sony.com>
> Cc: llvm-commits at lists.llvm.org
> Subject: Re: [llvm] ff837aa - Actually, don't try to use __builtin_strlen in StringRef.h before VS 2019
>
> Great, I'll just change the _MSC_VER expectation, then.
>
> Done in 1b3d1661bbeb2c90f8f3ef6e2b77a70bc148731e
>
> Can you confirm that it builds for you? I'll then cherry-pick it to 10.x.
>
> On Thu, Feb 6, 2020 at 12:01 PM Yung, Douglas <douglas.yung at sony.com> wrote:
> >
> > Yes, it works both with and without the constexpr.
> >
> > Douglas Yung
> >
> > -----Original Message-----
> > From: Hans Wennborg <hans at chromium.org>
> > Sent: Thursday, February 6, 2020 2:51
> > To: Yung, Douglas <douglas.yung at sony.com>
> > Cc: llvm-commits at lists.llvm.org
> > Subject: Re: [llvm] ff837aa - Actually, don't try to use
> > __builtin_strlen in StringRef.h before VS 2019
> >
> > Does it work also if you don't add the constexpr? That wasn't there in Reid's change.
> >
> > On Thu, Feb 6, 2020 at 11:39 AM Yung, Douglas <douglas.yung at sony.com> wrote:
> > >
> > > Hi Hans,
> > >
> > > Our build bot has Version 19.16.27027.1 installed, and I have
> > > Version
> > > 19.16.27035.0 on my local machine. I don't know when the builtin was
> > > added, but checking for >= 1916 instead of 1920 and adding the
> > > constexpr seems to work on my machine. (It builds/links)
> > >
> > > Douglas Yung
> > >
> > > -----Original Message-----
> > > From: Hans Wennborg <hans at chromium.org>
> > > Sent: Thursday, February 6, 2020 1:51
> > > To: Yung, Douglas <douglas.yung at sony.com>
> > > Cc: llvm-commits at lists.llvm.org
> > > Subject: Re: [llvm] ff837aa - Actually, don't try to use
> > > __builtin_strlen in StringRef.h before VS 2019
> > >
> > > Sorry about that. I'm using this version:
> > >
> > > Microsoft (R) C/C++ Optimizing Compiler Version 19.12.25834 for x64 Copyright (C) Microsoft Corporation.  All rights reserved.
> > >
> > > Which version are you using? It's possible that MSVC made __builtin_strlen work in constexpr contexts in a version later than the one I have, and then we can just tweak the _MSC_VER check.
> > >
> > > On Thu, Feb 6, 2020 at 10:15 AM Yung, Douglas <douglas.yung at sony.com> wrote:
> > > >
> > > > Hi Hans,
> > > >
> > > > What version of VS2017 are you using? We use VS2017 on our internal Windows build bot, and your change has caused a big regression in the compile time on our build bot. PR43369 has details on the compile time regression and the fix that Reid checked in here that you disabled for VS2017. We would really like to re-enable this for VS2017 if possible since it should and seemed to be working.
> > > >
> > > > Douglas Yung
> > > >
> > > > -----Original Message-----
> > > > From: llvm-commits <llvm-commits-bounces at lists.llvm.org> On Behalf
> > > > Of Hans Wennborg via llvm-commits
> > > > Sent: Monday, February 3, 2020 8:50
> > > > To: llvm-commits at lists.llvm.org
> > > > Subject: [llvm] ff837aa - Actually, don't try to use
> > > > __builtin_strlen in StringRef.h before VS 2019
> > > >
> > > >
> > > > Author: Hans Wennborg
> > > > Date: 2020-02-03T17:49:29+01:00
> > > > New Revision: ff837aa63cdfadb58f387ca77785ca3ca51c6976
> > > >
> > > > URL:
> > > > https://github.com/llvm/llvm-project/commit/ff837aa63cdfadb58f387c
> > > > a7
> > > > 77
> > > > 85ca3ca51c6976
> > > > DIFF:
> > > > https://github.com/llvm/llvm-project/commit/ff837aa63cdfadb58f387c
> > > > a7
> > > > 77
> > > > 85ca3ca51c6976.diff
> > > >
> > > > LOG: Actually, don't try to use __builtin_strlen in StringRef.h
> > > > before VS 2019
> > > >
> > > > The fix in b3d7d1061dc375bb5ea725e6597382fcd37f41d6 compiled nicely, but didn't link because at least the VS 2017 version I use doesn't have the builtin yet. Instead, make use of the builtin with MSVC conditional on VS 2019 or later.
> > > >
> > > > Added:
> > > >
> > > >
> > > > Modified:
> > > >     llvm/include/llvm/ADT/StringRef.h
> > > >
> > > > Removed:
> > > >
> > > >
> > > >
> > > > ##################################################################
> > > > ## ## ########## diff  --git a/llvm/include/llvm/ADT/StringRef.h
> > > > b/llvm/include/llvm/ADT/StringRef.h
> > > > index 31b0a5c6780f..75a6e1a6b5a8 100644
> > > > --- a/llvm/include/llvm/ADT/StringRef.h
> > > > +++ b/llvm/include/llvm/ADT/StringRef.h
> > > > @@ -27,7 +27,7 @@
> > > >  // Declare the __builtin_strlen intrinsic for MSVC so it can be used in  // constexpr context.
> > > >  #if defined(_MSC_VER)
> > > > -extern "C" constexpr size_t __builtin_strlen(const char *);
> > > > +extern "C" size_t __builtin_strlen(const char *);
> > > >  #endif
> > > >
> > > >  namespace llvm {
> > > > @@ -80,7 +80,8 @@ namespace llvm {
> > > >      static constexpr size_t strLen(const char *Str) {  #if __cplusplus > 201402L
> > > >        return std::char_traits<char>::length(Str);
> > > > -#elif __has_builtin(__builtin_strlen) || defined(__GNUC__) ||
> > > > defined(_MSC_VER)
> > > > +#elif __has_builtin(__builtin_strlen) || defined(__GNUC__) || \
> > > > +    (defined(_MSC_VER) && _MSC_VER >= 1920)
> > > >        return __builtin_strlen(Str);  #else
> > > >        const char *Begin = Str;
> > > >
> > > >
> > > >
> > > > _______________________________________________
> > > > llvm-commits mailing list
> > > > llvm-commits at lists.llvm.org
> > > > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits