r355698 - Re-fix _lrotl/_lrotr to always take Long, no matter the platform.
Michael Spencer via cfe-commits
cfe-commits at lists.llvm.org
Thu Feb 27 10:58:41 PST 2020
On Wed, Feb 26, 2020 at 8:39 PM James Y Knight <jyknight at google.com> wrote:
> I'm not clear as to what you're saying is broken.
>
> On MS platforms, long is 32-bit, so either way this function takes a 32bit
> value, right? And, Microsoft's docs say this function takes a long.
>
As per the original commit that changed this:
Support MS builtins using 'long' on LP64 platforms
>
This allows for -fms-extensions to work the same on LP64. For example,
> _BitScanReverse is expected to be 32-bit, matching Windows/LLP64, even
> though long is 64-bit on x86_64 Darwin or Linux (LP64).
>
Implement this by adding a new character code 'N', which is 'int' if
> the target is LP64 and the same 'L' otherwise
>
Differential Revision: https://reviews.llvm.org/D34377
The point of this change was to aid porting Windows code to LP64 targets
when using -fms-extensions.
In the future I would ask that before changing something that has an
explicit, documented test like this that you ping the original author for
review.
- Michael Spencer
>
>
> On Wed, Feb 26, 2020, 5:09 PM Michael Spencer via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> I believe this commit is wrong. These intrinsics were originally
>> implemented to support Windows code which expects _lrot{l,r}'s first
>> argument to be 32 bits, it's a breaking change to change these definitions.
>> I'm fine with adding the Intel version of these intrinsics, but you can't
>> just break the Microsoft version of them.
>>
>> - Michael Spencer
>>
>> On Fri, Mar 8, 2019 at 7:09 AM Erich Keane via cfe-commits <
>> cfe-commits at lists.llvm.org> wrote:
>>
>>> Author: erichkeane
>>> Date: Fri Mar 8 07:10:07 2019
>>> New Revision: 355698
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=355698&view=rev
>>> Log:
>>> Re-fix _lrotl/_lrotr to always take Long, no matter the platform.
>>>
>>> r355322 fixed this, however is being reverted due to concerns with
>>> enabling it in other modes.
>>>
>>> Change-Id: I6a939b7469b8fa196d5871a627eb2330dbd30f29
>>>
>>> Modified:
>>> cfe/trunk/include/clang/Basic/Builtins.def
>>> cfe/trunk/test/CodeGen/ms-intrinsics-rotations.c
>>>
>>> Modified: cfe/trunk/include/clang/Basic/Builtins.def
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Builtins.def?rev=355698&r1=355697&r2=355698&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/Basic/Builtins.def (original)
>>> +++ cfe/trunk/include/clang/Basic/Builtins.def Fri Mar 8 07:10:07 2019
>>> @@ -831,12 +831,12 @@ LANGBUILTIN(_ReturnAddress, "v*", "n", A
>>> LANGBUILTIN(_rotl8, "UcUcUc", "n", ALL_MS_LANGUAGES)
>>> LANGBUILTIN(_rotl16, "UsUsUc", "n", ALL_MS_LANGUAGES)
>>> LANGBUILTIN(_rotl, "UiUii", "n", ALL_MS_LANGUAGES)
>>> -LANGBUILTIN(_lrotl, "UNiUNii", "n", ALL_MS_LANGUAGES)
>>> +LANGBUILTIN(_lrotl, "ULiULii", "n", ALL_MS_LANGUAGES)
>>> LANGBUILTIN(_rotl64, "UWiUWii", "n", ALL_MS_LANGUAGES)
>>> LANGBUILTIN(_rotr8, "UcUcUc", "n", ALL_MS_LANGUAGES)
>>> LANGBUILTIN(_rotr16, "UsUsUc", "n", ALL_MS_LANGUAGES)
>>> LANGBUILTIN(_rotr, "UiUii", "n", ALL_MS_LANGUAGES)
>>> -LANGBUILTIN(_lrotr, "UNiUNii", "n", ALL_MS_LANGUAGES)
>>> +LANGBUILTIN(_lrotr, "ULiULii", "n", ALL_MS_LANGUAGES)
>>> LANGBUILTIN(_rotr64, "UWiUWii", "n", ALL_MS_LANGUAGES)
>>> LANGBUILTIN(__va_start, "vc**.", "nt", ALL_MS_LANGUAGES)
>>> LANGBUILTIN(__fastfail, "vUi", "nr", ALL_MS_LANGUAGES)
>>>
>>> Modified: cfe/trunk/test/CodeGen/ms-intrinsics-rotations.c
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/ms-intrinsics-rotations.c?rev=355698&r1=355697&r2=355698&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/test/CodeGen/ms-intrinsics-rotations.c (original)
>>> +++ cfe/trunk/test/CodeGen/ms-intrinsics-rotations.c Fri Mar 8 07:10:07
>>> 2019
>>> @@ -12,17 +12,10 @@
>>> // RUN: | FileCheck %s --check-prefixes CHECK,CHECK-32BIT-LONG
>>> // RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility
>>> -fms-compatibility-version=17.00 \
>>> // RUN: -triple x86_64--linux -emit-llvm %s -o - \
>>> -// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-32BIT-LONG
>>> +// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-64BIT-LONG
>>> // RUN: %clang_cc1 -ffreestanding -fms-extensions \
>>> // RUN: -triple x86_64--darwin -emit-llvm %s -o - \
>>> -// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-32BIT-LONG
>>> -
>>> -// LP64 targets use 'long' as 'int' for MS intrinsics (-fms-extensions)
>>> -#ifdef __LP64__
>>> -#define LONG int
>>> -#else
>>> -#define LONG long
>>> -#endif
>>> +// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-64BIT-LONG
>>>
>>> // rotate left
>>>
>>> @@ -47,12 +40,15 @@ unsigned int test_rotl(unsigned int valu
>>> // CHECK: [[R:%.*]] = call i32 @llvm.fshl.i32(i32 [[X:%.*]], i32
>>> [[X]], i32 [[Y:%.*]])
>>> // CHECK: ret i32 [[R]]
>>>
>>> -unsigned LONG test_lrotl(unsigned LONG value, int shift) {
>>> +unsigned long test_lrotl(unsigned long value, int shift) {
>>> return _lrotl(value, shift);
>>> }
>>> // CHECK-32BIT-LONG: i32 @test_lrotl
>>> // CHECK-32BIT-LONG: [[R:%.*]] = call i32 @llvm.fshl.i32(i32
>>> [[X:%.*]], i32 [[X]], i32 [[Y:%.*]])
>>> // CHECK-32BIT-LONG: ret i32 [[R]]
>>> +// CHECK-64BIT-LONG: i64 @test_lrotl
>>> +// CHECK-64BIT-LONG: [[R:%.*]] = call i64 @llvm.fshl.i64(i64
>>> [[X:%.*]], i64 [[X]], i64 [[Y:%.*]])
>>> +// CHECK-64BIT-LONG: ret i64 [[R]]
>>>
>>> unsigned __int64 test_rotl64(unsigned __int64 value, int shift) {
>>> return _rotl64(value, shift);
>>> @@ -84,12 +80,15 @@ unsigned int test_rotr(unsigned int valu
>>> // CHECK: [[R:%.*]] = call i32 @llvm.fshr.i32(i32 [[X:%.*]], i32
>>> [[X]], i32 [[Y:%.*]])
>>> // CHECK: ret i32 [[R]]
>>>
>>> -unsigned LONG test_lrotr(unsigned LONG value, int shift) {
>>> +unsigned long test_lrotr(unsigned long value, int shift) {
>>> return _lrotr(value, shift);
>>> }
>>> // CHECK-32BIT-LONG: i32 @test_lrotr
>>> // CHECK-32BIT-LONG: [[R:%.*]] = call i32 @llvm.fshr.i32(i32
>>> [[X:%.*]], i32 [[X]], i32 [[Y:%.*]])
>>> // CHECK-32BIT-LONG: ret i32 [[R]]
>>> +// CHECK-64BIT-LONG: i64 @test_lrotr
>>> +// CHECK-64BIT-LONG: [[R:%.*]] = call i64 @llvm.fshr.i64(i64
>>> [[X:%.*]], i64 [[X]], i64 [[Y:%.*]])
>>> +// CHECK-64BIT-LONG: ret i64 [[R]]
>>>
>>> unsigned __int64 test_rotr64(unsigned __int64 value, int shift) {
>>> return _rotr64(value, shift);
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200227/3804bb55/attachment-0001.html>
More information about the cfe-commits
mailing list