[PATCH] D37448: Fix cast assertion on MS inline assembly with vector spills (PR34021)
This revision was automatically updated to reflect the committed changes.
Closed by commit rL313019: [MS-InlineAsm] Fix cast assertion with vector spills
(PR34021) (authored by RKSimon).
Changed prior to commit:
https://reviews.llvm.org/D37448?vs=114569=114794#toc
Repository:
rL LLVM
https://reviews.llvm.org/D37448
Files:
cfe/trunk/lib/CodeGen/CGStmt.cpp
cfe/trunk/test/CodeGen/pr34021.c
Index: cfe/trunk/lib/CodeGen/CGStmt.cpp
===
--- cfe/trunk/lib/CodeGen/CGStmt.cpp
+++ cfe/trunk/lib/CodeGen/CGStmt.cpp
@@ -2194,7 +2194,7 @@
llvm::IntegerType::get(getLLVMContext(),
(unsigned)TmpSize));
Tmp = Builder.CreateTrunc(Tmp, TruncTy);
} else if (TruncTy->isIntegerTy()) {
-Tmp = Builder.CreateTrunc(Tmp, TruncTy);
+Tmp = Builder.CreateZExtOrTrunc(Tmp, TruncTy);
} else if (TruncTy->isVectorTy()) {
Tmp = Builder.CreateBitCast(Tmp, TruncTy);
}
Index: cfe/trunk/test/CodeGen/pr34021.c
===
--- cfe/trunk/test/CodeGen/pr34021.c
+++ cfe/trunk/test/CodeGen/pr34021.c
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -fms-extensions %s -triple=i686-unknown-unknown -emit-llvm
-o - | FileCheck %s --check-prefix=X86
+// RUN: %clang_cc1 -fms-extensions %s -triple=x86_64-unknown-unknown
-emit-llvm -o - | FileCheck %s --check-prefix=X64
+
+typedef int v4si __attribute__ ((vector_size (16)));
+v4si rep() {
+// X86-LABEL: define <4 x i32> @rep
+// X86: %retval = alloca <4 x i32>, align 16
+// X86-NEXT: %res = alloca <4 x i32>, align 16
+// X86-NEXT: %0 = bitcast <4 x i32>* %retval to i128*
+// X86-NEXT: %1 = call i64 asm sideeffect inteldialect "",
"=A,~{dirflag},~{fpsr},~{flags}"()
+// X86-NEXT: %2 = zext i64 %1 to i128
+// X86-NEXT: store i128 %2, i128* %0, align 16
+// X86-NEXT: %3 = load <4 x i32>, <4 x i32>* %res, align 16
+// X86-NEXT: ret <4 x i32> %3
+//
+// X64-LABEL: define <4 x i32> @rep
+// X64: %res = alloca <4 x i32>, align 16
+// X64-NEXT: call void asm sideeffect inteldialect "",
"~{dirflag},~{fpsr},~{flags}"()
+// X64-NEXT: %0 = load <4 x i32>, <4 x i32>* %res, align 16
+// X64-NEXT: ret <4 x i32> %0
+ v4si res;
+ __asm {}
+ return res;
+}
Index: cfe/trunk/lib/CodeGen/CGStmt.cpp
===
--- cfe/trunk/lib/CodeGen/CGStmt.cpp
+++ cfe/trunk/lib/CodeGen/CGStmt.cpp
@@ -2194,7 +2194,7 @@
llvm::IntegerType::get(getLLVMContext(), (unsigned)TmpSize));
Tmp = Builder.CreateTrunc(Tmp, TruncTy);
} else if (TruncTy->isIntegerTy()) {
-Tmp = Builder.CreateTrunc(Tmp, TruncTy);
+Tmp = Builder.CreateZExtOrTrunc(Tmp, TruncTy);
} else if (TruncTy->isVectorTy()) {
Tmp = Builder.CreateBitCast(Tmp, TruncTy);
}
Index: cfe/trunk/test/CodeGen/pr34021.c
===
--- cfe/trunk/test/CodeGen/pr34021.c
+++ cfe/trunk/test/CodeGen/pr34021.c
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -fms-extensions %s -triple=i686-unknown-unknown -emit-llvm -o - | FileCheck %s --check-prefix=X86
+// RUN: %clang_cc1 -fms-extensions %s -triple=x86_64-unknown-unknown -emit-llvm -o - | FileCheck %s --check-prefix=X64
+
+typedef int v4si __attribute__ ((vector_size (16)));
+v4si rep() {
+// X86-LABEL: define <4 x i32> @rep
+// X86: %retval = alloca <4 x i32>, align 16
+// X86-NEXT: %res = alloca <4 x i32>, align 16
+// X86-NEXT: %0 = bitcast <4 x i32>* %retval to i128*
+// X86-NEXT: %1 = call i64 asm sideeffect inteldialect "", "=A,~{dirflag},~{fpsr},~{flags}"()
+// X86-NEXT: %2 = zext i64 %1 to i128
+// X86-NEXT: store i128 %2, i128* %0, align 16
+// X86-NEXT: %3 = load <4 x i32>, <4 x i32>* %res, align 16
+// X86-NEXT: ret <4 x i32> %3
+//
+// X64-LABEL: define <4 x i32> @rep
+// X64: %res = alloca <4 x i32>, align 16
+// X64-NEXT: call void asm sideeffect inteldialect "", "~{dirflag},~{fpsr},~{flags}"()
+// X64-NEXT: %0 = load <4 x i32>, <4 x i32>* %res, align 16
+// X64-NEXT: ret <4 x i32> %0
+ v4si res;
+ __asm {}
+ return res;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37448: Fix cast assertion on MS inline assembly with vector spills (PR34021)
RKSimon added a comment. In https://reviews.llvm.org/D37448#866700, @rnk wrote: > lgtm > > I was hoping for a test case that didn't require assertions, but this is > enough to land the fix. My mistake, I should be able to remove that as well - I'll do it as part of the commit. Thanks. Repository: rL LLVM https://reviews.llvm.org/D37448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37448: Fix cast assertion on MS inline assembly with vector spills (PR34021)
rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm I was hoping for a test case that didn't require assertions, but this is enough to land the fix. Repository: rL LLVM https://reviews.llvm.org/D37448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37448: Fix cast assertion on MS inline assembly with vector spills (PR34021)
RKSimon updated this revision to Diff 114569.
RKSimon added a comment.
Added checks to test case
Repository:
rL LLVM
https://reviews.llvm.org/D37448
Files:
lib/CodeGen/CGStmt.cpp
test/CodeGen/pr34021.c
Index: test/CodeGen/pr34021.c
===
--- test/CodeGen/pr34021.c
+++ test/CodeGen/pr34021.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -fms-extensions %s -triple=i686-unknown-unknown -emit-llvm
-o - | FileCheck %s --check-prefix=X86
+// RUN: %clang_cc1 -fms-extensions %s -triple=x86_64-unknown-unknown
-emit-llvm -o - | FileCheck %s --check-prefix=X64
+// REQUIRES: asserts
+
+typedef int v4si __attribute__ ((vector_size (16)));
+v4si rep() {
+// X86-LABEL: define <4 x i32> @rep
+// X86: %retval = alloca <4 x i32>, align 16
+// X86-NEXT: %res = alloca <4 x i32>, align 16
+// X86-NEXT: %0 = bitcast <4 x i32>* %retval to i128*
+// X86-NEXT: %1 = call i64 asm sideeffect inteldialect "",
"=A,~{dirflag},~{fpsr},~{flags}"()
+// X86-NEXT: %2 = zext i64 %1 to i128
+// X86-NEXT: store i128 %2, i128* %0, align 16
+// X86-NEXT: %3 = load <4 x i32>, <4 x i32>* %res, align 16
+// X86-NEXT: ret <4 x i32> %3
+//
+// X64-LABEL: define <4 x i32> @rep
+// X64: %res = alloca <4 x i32>, align 16
+// X64-NEXT: call void asm sideeffect inteldialect "",
"~{dirflag},~{fpsr},~{flags}"()
+// X64-NEXT: %0 = load <4 x i32>, <4 x i32>* %res, align 16
+// X64-NEXT: ret <4 x i32> %0
+ v4si res;
+ __asm {}
+ return res;
+}
Index: lib/CodeGen/CGStmt.cpp
===
--- lib/CodeGen/CGStmt.cpp
+++ lib/CodeGen/CGStmt.cpp
@@ -2194,7 +2194,7 @@
llvm::IntegerType::get(getLLVMContext(),
(unsigned)TmpSize));
Tmp = Builder.CreateTrunc(Tmp, TruncTy);
} else if (TruncTy->isIntegerTy()) {
-Tmp = Builder.CreateTrunc(Tmp, TruncTy);
+Tmp = Builder.CreateZExtOrTrunc(Tmp, TruncTy);
} else if (TruncTy->isVectorTy()) {
Tmp = Builder.CreateBitCast(Tmp, TruncTy);
}
Index: test/CodeGen/pr34021.c
===
--- test/CodeGen/pr34021.c
+++ test/CodeGen/pr34021.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -fms-extensions %s -triple=i686-unknown-unknown -emit-llvm -o - | FileCheck %s --check-prefix=X86
+// RUN: %clang_cc1 -fms-extensions %s -triple=x86_64-unknown-unknown -emit-llvm -o - | FileCheck %s --check-prefix=X64
+// REQUIRES: asserts
+
+typedef int v4si __attribute__ ((vector_size (16)));
+v4si rep() {
+// X86-LABEL: define <4 x i32> @rep
+// X86: %retval = alloca <4 x i32>, align 16
+// X86-NEXT: %res = alloca <4 x i32>, align 16
+// X86-NEXT: %0 = bitcast <4 x i32>* %retval to i128*
+// X86-NEXT: %1 = call i64 asm sideeffect inteldialect "", "=A,~{dirflag},~{fpsr},~{flags}"()
+// X86-NEXT: %2 = zext i64 %1 to i128
+// X86-NEXT: store i128 %2, i128* %0, align 16
+// X86-NEXT: %3 = load <4 x i32>, <4 x i32>* %res, align 16
+// X86-NEXT: ret <4 x i32> %3
+//
+// X64-LABEL: define <4 x i32> @rep
+// X64: %res = alloca <4 x i32>, align 16
+// X64-NEXT: call void asm sideeffect inteldialect "", "~{dirflag},~{fpsr},~{flags}"()
+// X64-NEXT: %0 = load <4 x i32>, <4 x i32>* %res, align 16
+// X64-NEXT: ret <4 x i32> %0
+ v4si res;
+ __asm {}
+ return res;
+}
Index: lib/CodeGen/CGStmt.cpp
===
--- lib/CodeGen/CGStmt.cpp
+++ lib/CodeGen/CGStmt.cpp
@@ -2194,7 +2194,7 @@
llvm::IntegerType::get(getLLVMContext(), (unsigned)TmpSize));
Tmp = Builder.CreateTrunc(Tmp, TruncTy);
} else if (TruncTy->isIntegerTy()) {
-Tmp = Builder.CreateTrunc(Tmp, TruncTy);
+Tmp = Builder.CreateZExtOrTrunc(Tmp, TruncTy);
} else if (TruncTy->isVectorTy()) {
Tmp = Builder.CreateBitCast(Tmp, TruncTy);
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37448: Fix cast assertion on MS inline assembly with vector spills (PR34021)
erichkeane added a comment. In https://reviews.llvm.org/D37448#861443, @efriedma wrote: > > According to our definition, v4si is NOT a "Vector" type, since a vector > > type requires it be a FP value. > > Umm, what? An integer vector is still a vector. The backend will return it > in xmm0 on x86 (both 32 and 64-bit). Ugg.. you're right. I missed that addReturnRegisterOutputs was creating a return type as an integer, so when it was being checked in this if/else tree (edited above) you're not actually looking at the return type anymore Repository: rL LLVM https://reviews.llvm.org/D37448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37448: Fix cast assertion on MS inline assembly with vector spills (PR34021)
efriedma added a comment. > According to our definition, v4si is NOT a "Vector" type, since a vector type > requires it be a FP value. Umm, what? An integer vector is still a vector. The backend will return it in xmm0 on x86 (both 32 and 64-bit). The actual problem here is that X86_32TargetCodeGenInfo::addReturnRegisterOutputs is adding outputs which don't make any sense; 32-bit x86 only returns integers and pointers (and maybe a few other weird things like complex integers and tiny vectors?) in EAX:EDX. So we shouldn't add a constraint which involves those registers if the function returns a 128-bit vector (which goes in xmm0), or a struct (which is returned in memory), or a float (which is returned in an x87 register), etc. Adding the "right" constraint for stuff that isn't returned in EAX would be nice, but probably isn't necessary unless we actually run into code which depends on it. Repository: rL LLVM https://reviews.llvm.org/D37448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37448: Fix cast assertion on MS inline assembly with vector spills (PR34021)
erichkeane added a comment. In https://reviews.llvm.org/D37448#861211, @rnk wrote: > I think this is a reasonable stop-gap fix since the code isn't trying to > return EAX:EDX or XMM0 from the inline asm blob. This affects any function > that contains inline asm and returns a vector, which is potentially a lot of > stuff. Actually... I've convinced myself that this is likely the correct fix for this bug. There is likely a separate bug for vector-types and struct return types, however I'd believe those aren't really things that anyone else supports. According to our definition, v4si is NOT a "Vector" type, since a vector type requires it be a FP value. SO, we are converting an integer stored in eax/edx to a wider integer. Since assembly doesn't have a 'signed'-ness, I would assert that widening a register-stored variable to via zero-ext is the correct fix here. In summary, I believe this is the correct fix for the bug as reported. Repository: rL LLVM https://reviews.llvm.org/D37448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37448: Fix cast assertion on MS inline assembly with vector spills (PR34021)
rnk added a comment.
I think this is a reasonable stop-gap fix since the code isn't trying to return
EAX:EDX or XMM0 from the inline asm blob. This affects any function that
contains inline asm and returns a vector, which is potentially a lot of stuff.
Comment at: test/CodeGen/pr34021.c:2
+// RUN: %clang_cc1 -fms-extensions %s -triple=i686-unknown-unknown -emit-llvm
-o -
+// RUN: %clang_cc1 -fms-extensions %s -triple=x86_64-unknown-unknown
-emit-llvm -o -
+// REQUIRES: asserts
Please FileCheck the IR. We should see a pattern like this:
```
define <4 x float> @rep()
%retval = alloca <4 x float>
call i64 inlineasm sideeffect {{.*}}
store {{.*}}, <4 x float>* %retval
%v = load {{.*}}, <4 x float>* %res
store <4 x float> %v, <4 x float>* %retval
%rv = load <4 x float>* %retval
ret <4 x float> %rv
```
That IR pattern makes it clear that the store of the asm output is immediately
killed by the store of `res` to `retval`.
Repository:
rL LLVM
https://reviews.llvm.org/D37448
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37448: Fix cast assertion on MS inline assembly with vector spills (PR34021)
erichkeane added a comment. Ah, I seem to remember that the problem here is that we don't know whether this value should be signed or unsigned at this point, so it could be CreateZExtOrTrunc OR CreateSignExtOrTrunc? However, there is no information as to whether the integer is signed here as far as I remember... Repository: rL LLVM https://reviews.llvm.org/D37448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37448: Fix cast assertion on MS inline assembly with vector spills (PR34021)
erichkeane added a comment. In https://reviews.llvm.org/D37448#861069, @RKSimon wrote: > In https://reviews.llvm.org/D37448#861019, @erichkeane wrote: > > > I apologize for the lack of detail for this comment, but I didn't seem to > > capture this at the time. When I investigated this initially, I came up > > with this solution and it was insufficient. I don't remember WHAT this > > ends up not fixing however. > > > @erichkeane OK, are you in a position to take on this bug? Either > commandeering this patch or I abandon it and you start your own? I'm not sure I have time for it unfortunately. It IS on my personal backlog, I just haven't had time lately. Has this affected someone else (besides my company's internal tests), or become more important lately? Repository: rL LLVM https://reviews.llvm.org/D37448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37448: Fix cast assertion on MS inline assembly with vector spills (PR34021)
RKSimon added a comment. In https://reviews.llvm.org/D37448#861019, @erichkeane wrote: > I apologize for the lack of detail for this comment, but I didn't seem to > capture this at the time. When I investigated this initially, I came up with > this solution and it was insufficient. I don't remember WHAT this ends up > not fixing however. @erichkeane OK, are you in a position to take on this bug? Either commandeering this patch or I abandon it and you start your own? Repository: rL LLVM https://reviews.llvm.org/D37448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37448: Fix cast assertion on MS inline assembly with vector spills (PR34021)
erichkeane added a comment. I apologize for the lack of detail for this comment, but I didn't seem to capture this at the time. When I investigated this initially, I came up with this solution and it was insufficient. I don't remember WHAT this ends up not fixing however. When discussing it with @rnk we decided that (according to my notes): "so, addReturnRegisterOutputs needs to look at the LLVM return type, and choose between EAX:EDX for integer / struct return types, ST0 for small FP types, and XMM0/YMM0 etc for vectors/big FP types" The issue is more that the *TargetInfo::addReturnRegisterOutputs doesn't properly configure things, it simply works with integers. It needs to be extended to work with int/struct types, as well as vector/FP types. Repository: rL LLVM https://reviews.llvm.org/D37448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37448: Fix cast assertion on MS inline assembly with vector spills (PR34021)
coby added a comment. might be a bit unrelated - but do we've got a hint regarding why is this even an issue? by all means - it doesn't seems right for an empty ms inline-asm statement to affect successful compilation, without even mentioning the involvement of the encapsulating function's return type. Another semi-adopted MS legacy issue? Repository: rL LLVM https://reviews.llvm.org/D37448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37448: Fix cast assertion on MS inline assembly with vector spills (PR34021)
RKSimon created this revision.
I can add the other test cases from PR34021 if required?
Repository:
rL LLVM
https://reviews.llvm.org/D37448
Files:
lib/CodeGen/CGStmt.cpp
test/CodeGen/pr34021.c
Index: test/CodeGen/pr34021.c
===
--- test/CodeGen/pr34021.c
+++ test/CodeGen/pr34021.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fms-extensions %s -triple=i686-unknown-unknown -emit-llvm
-o -
+// RUN: %clang_cc1 -fms-extensions %s -triple=x86_64-unknown-unknown
-emit-llvm -o -
+// REQUIRES: asserts
+
+typedef int v4si __attribute__ ((vector_size (16)));
+v4si rep() {
+v4si res;
+__asm {}
+return res;
+}
Index: lib/CodeGen/CGStmt.cpp
===
--- lib/CodeGen/CGStmt.cpp
+++ lib/CodeGen/CGStmt.cpp
@@ -2210,7 +2210,7 @@
llvm::IntegerType::get(getLLVMContext(),
(unsigned)TmpSize));
Tmp = Builder.CreateTrunc(Tmp, TruncTy);
} else if (TruncTy->isIntegerTy()) {
-Tmp = Builder.CreateTrunc(Tmp, TruncTy);
+Tmp = Builder.CreateZExtOrTrunc(Tmp, TruncTy);
} else if (TruncTy->isVectorTy()) {
Tmp = Builder.CreateBitCast(Tmp, TruncTy);
}
Index: test/CodeGen/pr34021.c
===
--- test/CodeGen/pr34021.c
+++ test/CodeGen/pr34021.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fms-extensions %s -triple=i686-unknown-unknown -emit-llvm -o -
+// RUN: %clang_cc1 -fms-extensions %s -triple=x86_64-unknown-unknown -emit-llvm -o -
+// REQUIRES: asserts
+
+typedef int v4si __attribute__ ((vector_size (16)));
+v4si rep() {
+v4si res;
+__asm {}
+return res;
+}
Index: lib/CodeGen/CGStmt.cpp
===
--- lib/CodeGen/CGStmt.cpp
+++ lib/CodeGen/CGStmt.cpp
@@ -2210,7 +2210,7 @@
llvm::IntegerType::get(getLLVMContext(), (unsigned)TmpSize));
Tmp = Builder.CreateTrunc(Tmp, TruncTy);
} else if (TruncTy->isIntegerTy()) {
-Tmp = Builder.CreateTrunc(Tmp, TruncTy);
+Tmp = Builder.CreateZExtOrTrunc(Tmp, TruncTy);
} else if (TruncTy->isVectorTy()) {
Tmp = Builder.CreateBitCast(Tmp, TruncTy);
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
