Fix cast assertion on MS inline assembly with vector spills (PR34021)

[RKSimon]

Summary

I can add the other test cases from PR34021 if required?

[coby]

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?

[erichkeane]

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.

[RKSimon]

In 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?

[erichkeane]

In D37448#861069, @RKSimon wrote:

In 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?

[erichkeane]

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...

[rnk]

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.

test/CodeGen/pr34021.c

2 ↗

(On Diff #113785)

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.

[erichkeane]

In 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.

[efriedma]

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.

[erichkeane]

In 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....

[RKSimon]

Added checks to test case

[rnk]

lgtm

I was hoping for a test case that didn't require assertions, but this is enough to land the fix.

[RKSimon]

In 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.

cfe/trunk/lib/CodeGen/CGStmt.cpp

if (ResultRegTypes[i] != ResultTruncRegTypes[i]) {
                    llvm::IntegerType::get(getLLVMContext(), (unsigned)ResSize));
         Tmp = Builder.CreateIntToPtr(Tmp, TruncTy);
       } else if (Tmp->getType()->isPointerTy() && TruncTy->isIntegerTy()) {
         uint64_t TmpSize =CGM.getDataLayout().getTypeSizeInBits(Tmp->getType());
         Tmp = Builder.CreatePtrToInt(Tmp,
                    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);
       }
     }
 
     EmitStoreThroughLValue(RValue::get(Tmp), ResultRegDests[i]);
   }
 }

cfe/trunk/test/CodeGen/pr34021.c

+// 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;
+}