<div dir="ltr"><div dir="ltr">On Wed, Feb 26, 2020 at 8:39 PM James Y Knight <<a href="mailto:jyknight@google.com">jyknight@google.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div>I'm not clear as to what you're saying is broken.</div><div dir="auto"><br></div><div dir="auto">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.</div></div></blockquote><div><br></div><div>As per the original commit that changed this:</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Support MS builtins using 'long' on LP64 platforms<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">This allows for -fms-extensions to work the same on LP64. For example,<br>_BitScanReverse is expected to be 32-bit, matching Windows/LLP64, even<br>though long is 64-bit on x86_64 Darwin or Linux (LP64).<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Implement this by adding a new character code 'N', which is 'int' if<br>the target is LP64 and the same 'L' otherwise<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Differential Revision: <a href="https://reviews.llvm.org/D34377">https://reviews.llvm.org/D34377</a></blockquote><div><br></div><div>The point of this change was to aid porting Windows code to LP64 targets when using -fms-extensions.</div><div><br></div><div>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.</div><div><br></div><div>- Michael Spencer<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="auto"><br></div><div dir="auto"><br><div class="gmail_quote" dir="auto"><div dir="ltr" class="gmail_attr">On Wed, Feb 26, 2020, 5:09 PM Michael Spencer via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" rel="noreferrer" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>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.</div><div dir="ltr"><br clear="all"><div><div dir="ltr">- Michael Spencer</div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Mar 8, 2019 at 7:09 AM Erich Keane via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" rel="noreferrer noreferrer" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: erichkeane<br>
Date: Fri Mar 8 07:10:07 2019<br>
New Revision: 355698<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=355698&view=rev" rel="noreferrer noreferrer noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=355698&view=rev</a><br>
Log:<br>
Re-fix _lrotl/_lrotr to always take Long, no matter the platform.<br>
<br>
r355322 fixed this, however is being reverted due to concerns with<br>
enabling it in other modes.<br>
<br>
Change-Id: I6a939b7469b8fa196d5871a627eb2330dbd30f29<br>
<br>
Modified:<br>
cfe/trunk/include/clang/Basic/Builtins.def<br>
cfe/trunk/test/CodeGen/ms-intrinsics-rotations.c<br>
<br>
Modified: cfe/trunk/include/clang/Basic/Builtins.def<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Builtins.def?rev=355698&r1=355697&r2=355698&view=diff" rel="noreferrer noreferrer noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Builtins.def?rev=355698&r1=355697&r2=355698&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/Builtins.def (original)<br>
+++ cfe/trunk/include/clang/Basic/Builtins.def Fri Mar 8 07:10:07 2019<br>
@@ -831,12 +831,12 @@ LANGBUILTIN(_ReturnAddress, "v*", "n", A<br>
LANGBUILTIN(_rotl8, "UcUcUc", "n", ALL_MS_LANGUAGES)<br>
LANGBUILTIN(_rotl16, "UsUsUc", "n", ALL_MS_LANGUAGES)<br>
LANGBUILTIN(_rotl, "UiUii", "n", ALL_MS_LANGUAGES)<br>
-LANGBUILTIN(_lrotl, "UNiUNii", "n", ALL_MS_LANGUAGES)<br>
+LANGBUILTIN(_lrotl, "ULiULii", "n", ALL_MS_LANGUAGES)<br>
LANGBUILTIN(_rotl64, "UWiUWii", "n", ALL_MS_LANGUAGES)<br>
LANGBUILTIN(_rotr8, "UcUcUc", "n", ALL_MS_LANGUAGES)<br>
LANGBUILTIN(_rotr16, "UsUsUc", "n", ALL_MS_LANGUAGES)<br>
LANGBUILTIN(_rotr, "UiUii", "n", ALL_MS_LANGUAGES)<br>
-LANGBUILTIN(_lrotr, "UNiUNii", "n", ALL_MS_LANGUAGES)<br>
+LANGBUILTIN(_lrotr, "ULiULii", "n", ALL_MS_LANGUAGES)<br>
LANGBUILTIN(_rotr64, "UWiUWii", "n", ALL_MS_LANGUAGES)<br>
LANGBUILTIN(__va_start, "vc**.", "nt", ALL_MS_LANGUAGES)<br>
LANGBUILTIN(__fastfail, "vUi", "nr", ALL_MS_LANGUAGES)<br>
<br>
Modified: cfe/trunk/test/CodeGen/ms-intrinsics-rotations.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/ms-intrinsics-rotations.c?rev=355698&r1=355697&r2=355698&view=diff" rel="noreferrer noreferrer noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/ms-intrinsics-rotations.c?rev=355698&r1=355697&r2=355698&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/CodeGen/ms-intrinsics-rotations.c (original)<br>
+++ cfe/trunk/test/CodeGen/ms-intrinsics-rotations.c Fri Mar 8 07:10:07 2019<br>
@@ -12,17 +12,10 @@<br>
// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-32BIT-LONG<br>
// RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility -fms-compatibility-version=17.00 \<br>
// RUN: -triple x86_64--linux -emit-llvm %s -o - \<br>
-// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-32BIT-LONG<br>
+// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-64BIT-LONG<br>
// RUN: %clang_cc1 -ffreestanding -fms-extensions \<br>
// RUN: -triple x86_64--darwin -emit-llvm %s -o - \<br>
-// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-32BIT-LONG<br>
-<br>
-// LP64 targets use 'long' as 'int' for MS intrinsics (-fms-extensions)<br>
-#ifdef __LP64__<br>
-#define LONG int<br>
-#else<br>
-#define LONG long<br>
-#endif<br>
+// RUN: | FileCheck %s --check-prefixes CHECK,CHECK-64BIT-LONG<br>
<br>
// rotate left<br>
<br>
@@ -47,12 +40,15 @@ unsigned int test_rotl(unsigned int valu<br>
// CHECK: [[R:%.*]] = call i32 @llvm.fshl.i32(i32 [[X:%.*]], i32 [[X]], i32 [[Y:%.*]])<br>
// CHECK: ret i32 [[R]]<br>
<br>
-unsigned LONG test_lrotl(unsigned LONG value, int shift) {<br>
+unsigned long test_lrotl(unsigned long value, int shift) {<br>
return _lrotl(value, shift);<br>
}<br>
// CHECK-32BIT-LONG: i32 @test_lrotl<br>
// CHECK-32BIT-LONG: [[R:%.*]] = call i32 @llvm.fshl.i32(i32 [[X:%.*]], i32 [[X]], i32 [[Y:%.*]])<br>
// CHECK-32BIT-LONG: ret i32 [[R]]<br>
+// CHECK-64BIT-LONG: i64 @test_lrotl<br>
+// CHECK-64BIT-LONG: [[R:%.*]] = call i64 @llvm.fshl.i64(i64 [[X:%.*]], i64 [[X]], i64 [[Y:%.*]])<br>
+// CHECK-64BIT-LONG: ret i64 [[R]]<br>
<br>
unsigned __int64 test_rotl64(unsigned __int64 value, int shift) {<br>
return _rotl64(value, shift);<br>
@@ -84,12 +80,15 @@ unsigned int test_rotr(unsigned int valu<br>
// CHECK: [[R:%.*]] = call i32 @llvm.fshr.i32(i32 [[X:%.*]], i32 [[X]], i32 [[Y:%.*]])<br>
// CHECK: ret i32 [[R]]<br>
<br>
-unsigned LONG test_lrotr(unsigned LONG value, int shift) {<br>
+unsigned long test_lrotr(unsigned long value, int shift) {<br>
return _lrotr(value, shift);<br>
}<br>
// CHECK-32BIT-LONG: i32 @test_lrotr<br>
// CHECK-32BIT-LONG: [[R:%.*]] = call i32 @llvm.fshr.i32(i32 [[X:%.*]], i32 [[X]], i32 [[Y:%.*]])<br>
// CHECK-32BIT-LONG: ret i32 [[R]]<br>
+// CHECK-64BIT-LONG: i64 @test_lrotr<br>
+// CHECK-64BIT-LONG: [[R:%.*]] = call i64 @llvm.fshr.i64(i64 [[X:%.*]], i64 [[X]], i64 [[Y:%.*]])<br>
+// CHECK-64BIT-LONG: ret i64 [[R]]<br>
<br>
unsigned __int64 test_rotr64(unsigned __int64 value, int shift) {<br>
return _rotr64(value, shift);<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" rel="noreferrer noreferrer" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer noreferrer noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" rel="noreferrer noreferrer" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer noreferrer noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div></div>
</blockquote></div></div>