r284060 - Implement MS _BitScan intrinsics

Tue Jun 20 09:07:42 PDT 2017

LGTM, assuming "howLong" == 0 means 32-bit int. 😊

-----Original Message-----
From: Bruno Cardoso Lopes [mailto:bruno.cardoso at gmail.com] 
Sent: Monday, June 19, 2017 6:45 PM
To: Duncan P. N. Exon Smith <dexonsmith at apple.com>
Cc: Reid Kleckner <rnk at google.com>; Erik Schwiebert <eriksc at microsoft.com>; Brian Kelley <bkelley at microsoft.com>; Tomasz Kukielka <tkukielk at microsoft.com>; cfe-commits <cfe-commits at lists.llvm.org>
Subject: Re: r284060 - Implement MS _BitScan intrinsics

On Fri, Jun 16, 2017 at 5:08 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
> On Jun 16, 2017, at 11:02, Reid Kleckner <rnk at google.com> wrote:
>
> We should fix it.
>
>
> Agreed.
>
> We just need a new character code in the builtin function prototype
> encoding. Currently there is no encoding for a portable int32_t that
> magically becomes "long" on 32-bit Windows. The closest thing we have is
> 'W', which is used by Neon intrinsics to select between "long" and "long
> long". It was added in r202004.
>
>
> Yup, I saw the 'W' as well.  That's the same approach I was using in a patch
> yesterday morning (I picked lowercase L ('l'), but there may be a better
> choice).  Likely Bruno will follow up with a working patch soon (I hit some
> problems with tests and then got tied up).

Right, here it is: https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD34377&data=02%7C01%7Ceriksc%40microsoft.com%7C622ea4cd65f846cd1be708d4b77dfaa1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636335199097092827&sdata=abfnUWviZ01v%2B%2B43Q6vLvZoBk2QbawkzKNpayIgMJwY%3D&reserved=0

Let me know what you guys think!

>
> On Fri, Jun 16, 2017 at 10:28 AM, Erik Schwiebert <eriksc at microsoft.com>
> wrote:
>>
>> We (Office developers for Apple platforms at Microsoft) would prefer to
>> have the intrinsics work properly for LP64, as that will generate the most
>> optimal and efficient code. That said, we understand that this may be odd to
>> implement from the compiler's perspective and can work around it if the
>> intrinsics are not supported for ms-extensions+LP64. At the end of the day
>> we need the compiler to clearly adopt one of those two options, and not sit
>> somewhere in the middle as it currently does. We don't turn on any flags
>> other than -fms-extensions; we don’t attempt to define MSC_VER and we do
>> conditionalize code based on __clang__ so the code is aware it is being
>> compiled by clang vs MSVC.
>>
>> So I think we'd prefer what Duncan and Apple are offering to do, but if
>> the larger clang community thinks that is a bad idea and wants to explicitly
>> disable the intrinsics, we could live with that.
>>
>> Thanks,
>> Schwieb
>>
>> -----Original Message-----
>> From: Erik Schwiebert
>> Sent: Friday, June 16, 2017 8:49 AM
>> To: 'Bruno Cardoso Lopes' <bruno.cardoso at gmail.com>; Brian Kelley
>> <bkelley at microsoft.com>; Tomasz Kukielka <tkukielk at microsoft.com>
>> Cc: dexonsmith at apple.com; Reid Kleckner <rnk at google.com>; cfe-commits
>> <cfe-commits at lists.llvm.org>
>> Subject: RE: r284060 - Implement MS _BitScan intrinsics
>>
>> Adding Brian and Tomasz. I'm pretty sure we have the Windows SDK
>> intrinsics headers. I'm not sure which method we'd prefer, so I'll walk down
>> the hall and ask them. Tomasz is our header maestro (because we play crazy
>> #include_next games so we can use both Windows and macOS SDKs!)
>>
>> We'll get back to you ASAP.
>>
>> Schwieb
>>
>> -----Original Message-----
>> From: Bruno Cardoso Lopes [mailto:bruno.cardoso at gmail.com]
>> Sent: Thursday, June 15, 2017 4:41 PM
>> To: Erik Schwiebert <eriksc at microsoft.com>
>> Cc: dexonsmith at apple.com; Reid Kleckner <rnk at google.com>; cfe-commits
>> <cfe-commits at lists.llvm.org>
>> Subject: Re: r284060 - Implement MS _BitScan intrinsics
>>
>> On Tue, Jun 13, 2017 at 8:13 PM, Bruno Cardoso Lopes
>> <bruno.cardoso at gmail.com> wrote:
>> > On Mon, Jun 12, 2017 at 2:01 PM, Erik Schwiebert via cfe-commits
>> > <cfe-commits at lists.llvm.org> wrote:
>> >> SGTM too. Regarding Duncan's last question -- I can't think of any such
>> >> customer. :) If you all think the right thing for clang to do is to infer
>> >> LLP64 behavior on LP64 (Darwin) + ms_extensions, then that is fine with me!
>>
>> Thinking more about this; what if we mark such builtins as unsupported
>> for "LP64 (Darwin) + ms_extensions" and then you provide the
>> definitions via intrin.h (we can generate a compiler macro for this
>> scenario and conditionalize the include_next as we do for _MSC_VER)?
>> Do you use this header at all? Are there any other MS related flags
>> that get passed to the compiler?
>>
>> > SGTM as well!
>> >
>> >>
>> >> Thanks all!
>> >> Schwieb
>> >>
>> >> -----Original Message-----
>> >> From: dexonsmith at apple.com [mailto:dexonsmith at apple.com]
>> >> Sent: Monday, June 12, 2017 1:55 PM
>> >> To: Reid Kleckner <rnk at google.com>
>> >> Cc: Saleem Abdulrasool <compnerd at compnerd.org>; Albert Gutowski
>> >> <agutowski at google.com>; David Majnemer <david.majnemer at gmail.com>;
>> >> cfe-commits <cfe-commits at lists.llvm.org>; Erik Schwiebert
>> >> <eriksc at microsoft.com>
>> >> Subject: Re: r284060 - Implement MS _BitScan intrinsics
>> >>
>> >>
>> >>> On Jun 12, 2017, at 12:44, Reid Kleckner <rnk at google.com> wrote:
>> >>>
>> >>>> On Wed, Jun 7, 2017 at 7:31 PM, Saleem Abdulrasool
>> >>>> <compnerd at compnerd.org> wrote:
>> >>>> I'm worried about changing this signature all the time.  I suspect
>> >>>> that it will cause the following to be emitted for valid code:
>> >>>>
>> >>>> warning: incompatible pointer types passing 'unsigned long *' to
>> >>>> parameter of type 'unsigned int *' [-Wincompatible-pointer-types]
>> >>>>
>> >>>> Switching the signature on LP64 sounds much better to me.
>> >>>
>> >>> Right, we have to do this. It needs to be `long` on Windows.
>> >>
>> >> SGTM.  We'll go that way.
>> >
>> > +1 here!
>> >
>> >>> On Jun 8, 2017, at 12:21, Erik Schwiebert <eriksc at microsoft.com>
>> >>> wrote:
>> >>>
>> >>> It’s probably also better to not try to infer our weird desired
>> >>> behavior. It should probably be controlled by a specific driver directive,
>> >>> like “-fms-extensions-lp64-intrinsics” or something like that. Using a new
>> >>> directive means that nobody can accidentally get this behavior if they for
>> >>> some reason do want LLP64 behavior with Windows intrinsics.
>> >>
>> >> This seems overly complicated.  Is there a customer that:
>> >> - is on LP64,
>> >> - is using -fms-extensions,
>> >> - is using these intrinsics, and
>> >> - wants them to be 64-bit longs instead of 32-bit ints?
>> >> Put another way: who would use these intrinsics on LP64 and *not* want
>> >> to mimic LLP64?
>> >>
>> >> If everyone using the intrinsics on LP64 is going to have to specify
>> >> -fms-extensions-lp64-intrinsics, then we should just imply it.
>> >> _______________________________________________
>> >> cfe-commits mailing list
>> >> cfe-commits at lists.llvm.org
>> >>
>> >> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.llvm.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fcfe-commits&data=02%7C01%7Ceriksc%40microsoft.com%7Cba4835eb4a1b438793d508d4b4481355%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636331669036098946&sdata=iWTF3jpX71tU%2B6aq%2BEpv8VD8IFfeDKMHvZd40%2FK64aE%3D&reserved=0
>> >
>> >
>> >
>> > --
>> > Bruno Cardoso Lopes
>> >
>> > https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.brunocardoso.cc&data=02%7C01%7Ceriksc%40microsoft.com%7Cba4835eb4a1b438793d508d4b4481355%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636331669036098946&sdata=O01DYn4W3JN54%2B6wwAgCLAkq63PUtUBy4mQ9RMN833s%3D&reserved=0
>>
>>
>>
>> --
>> Bruno Cardoso Lopes
>>
>> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.brunocardoso.cc&data=02%7C01%7Ceriksc%40microsoft.com%7Cba4835eb4a1b438793d508d4b4481355%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636331669036098946&sdata=O01DYn4W3JN54%2B6wwAgCLAkq63PUtUBy4mQ9RMN833s%3D&reserved=0
>
>
>

-- 
Bruno Cardoso Lopes
https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.brunocardoso.cc&data=02%7C01%7Ceriksc%40microsoft.com%7C622ea4cd65f846cd1be708d4b77dfaa1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636335199097092827&sdata=KkrMAt3i%2BEMst96yRWX9hlsQtbPFMwWS%2FbQ9FoBcGcU%3D&reserved=0