On Wed, Feb 26, 2020 at 8:39 PM James Y Knight <jykni...@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@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@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@lists.llvm.org >>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits