[Mono][Win][Amd] Incorrect decomposition for simd calls

[saitama951]

Program.txt

The attached program is an extracted algorithm from one of the libraries from dotnet.
This throws an assertion failure on Mono + amd + windows.

D:\exp\vector>dotnet bin/Debug/net10.0/vector.dll
* Assertion at D:\exp\runtime\src\mono\mono\mini\mini-codegen.c:1267, condition `reginfo [sreg].born_in > 0' not met

dotnet --info:

D:\exp\vector>dotnet --info
.NET SDK:
 Version:           10.0.100-preview.3.25201.16
 Commit:            9dee3cf508
 Workload version:  10.0.100-manifests.ef4f854f
 MSBuild version:   17.14.0-preview-25177-05+903614e16

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19044
 OS Platform: Windows
 RID:         win-x64
 Base Path:   D:\exp\dotnet-win\sdk\10.0.100-preview.3.25201.16\

[saitama951]

I have managed to get the verbose logs and here is my analysis:

when I look into the verbose logs I have this

    66 vcall2 [System.Runtime.Intrinsics.Vector128`1<single> System.Runtime.Intrinsics.Vector128:Min<single> (System.Runtime.Intrinsics.Vector128`1<single>,System.Runtime.Intrinsics.Vector128`1<single>)] [%rcx <- R51] [%r8 <- R140] [%rdx <- R146] clobbers: c
    67 load_membase R148 <- [%rbp + 0xfffffef8]
    68 storex_membase [R148] <- R147 [System.Runtime.Intrinsics.Vector128`1<single>]
* Assertion at D:\exp\runtime\src\mono\mono\mini\mini-codegen.c:1267, condition `reginfo [sreg].born_in > 0' not met

before decomposition we of Vector128.Min call we have this,

xcall R17 <- [System.Runtime.Intrinsics.Vector128`1<single> System.Runtime.Intrinsics.Vector128:Min<single> (System.Runtime.Intrinsics.Vector128`1<single>,System.Runtime.Intrinsics.Vector128`1<single>)] [%rcx <- R51]
 il_seq_point il: 0x45, nonempty-stack
 il_seq_point il: 0x47
 xmove R52 <- R17 [System.Runtime.Intrinsics.Vector128`1<single>]
 xconst R54 <-
 il_seq_point il: 0x52, nonempty-stack
 xcast R55 <- R54

and after the decomposition we decompose the xcall opcode to vcall2

vcall2 [System.Runtime.Intrinsics.Vector128`1<single> System.Runtime.Intrinsics.Vector128:Min<single> (System.Runtime.Intrinsics.Vector128`1<single>,System.Runtime.Intrinsics.Vector128`1<single>)] [%rcx <- R51] [%r8 <- R140] [%rdx <- R146] clobbers: c
 ldaddr R148 <- R17
 storex_membase [R148] <- R147 [System.Runtime.Intrinsics.Vector128`1<single>]
 il_seq_point il: 0x45, nonempty-stack
 il_seq_point il: 0x47
 ldaddr R149 <- R17
 loadx_membase R150 <- [R149 + 0x0] [System.Runtime.Intrinsics.Vector128`1<single>]
 xmove R52 <- R150 [System.Runtime.Intrinsics.Vector128`1<single>]
 xconst R54 <-
 il_seq_point il: 0x52, nonempty-stack
 xcast R55 <- R54

now from the above block this sequence makes sense where we try to load the address from the return register and load the vectors from it.

 ldaddr R149 <- R17
 loadx_membase R150 <- [R149 + 0x0] [System.Runtime.Intrinsics.Vector128`1<single>]

but the decomposition sequence above that doesn't really make sense to me (here R147 doesn't really have any context in this sequence hence the assertion failure?)

 ldaddr R148 <- R17
 storex_membase [R148] <- R147 [System.Runtime.Intrinsics.Vector128`1<single>]

note: here R17 is an arg vreg (so we basically rewrite the arg vreg when returning)
now looking at the code I think this above decomposition (ldaddr->storex_membase) is produced from here (https://github.com/dotnet/runtime/blob/main/src/mono/mono/mini/simd-intrinsics.c#L6716)
where we decompose OP_ARG types specially. I don't exactly understand how vcall2 works (I mean how return values are stored for this op) and do we need to special case this here for xcall opcodes?
since we already generate the right sequence after that i.e ldaddr -> loadx_membase

I have attached the mono verbose logs for reference

log-windows.log

[saitama951]

from the commit history I see a similar commit which describes the same problem before
d9ded8f

@lateralusX do you have any thoughts on this?

[lateralusX]

This looks familiar, believe I have fixed something similar in some downstream repositories at some point in time around xcalls, but I believe it was related to xcast as a result of returning from a xcall where SIMD value types are kept in return register. Let me see if I can find it and you can verify if it's indeed the same issue. Do you happen to know what instruction type it asserts on?

Windows x64 ABI will return a pointer to a stack allocated memory (passed in to the function in RCX) if the size of the struct is not 1,2,4,8 bytes in size and that cause some differences on how SIMD types gets handled since they will operate on the value being loaded into a SIMD register, so needs to be stored back into a SIMD register if returned back from a regular method call.

[lateralusX]

This is the fix to xcast, not sure it fix your issue, but worth trying out:

diff --git a/src/mono/mono/mini/simd-intrinsics.c b/src/mono/mono/mini/simd-intrinsics.c
index cefb5043bef..cdb692a8940 100644
--- a/src/mono/mono/mini/simd-intrinsics.c
+++ b/src/mono/mono/mini/simd-intrinsics.c
@@ -2145,27 +2145,28 @@ emit_sri_vector (MonoCompile *cfg, MonoMethod *cmethod, MonoMethodSignature *fsi
 	case SN_AsSingle:
 	case SN_AsUInt16:
 	case SN_AsUInt32:
-	case SN_AsUInt64: {
-		if (!is_element_type_primitive (fsig->ret) || !is_element_type_primitive (fsig->params [0]))
-			return NULL;
-		return emit_simd_ins (cfg, klass, OP_XCAST, args [0]->dreg, -1);
-	}
+	case SN_AsUInt64:
 	case SN_AsVector:
 	case SN_AsVector128:
 	case SN_AsVector4: {
 		if (!is_element_type_primitive (fsig->ret) || !is_element_type_primitive (fsig->params [0]))
 			return NULL;
 
-		MonoClass *ret_class = mono_class_from_mono_type_internal (fsig->ret);
-		int ret_size = mono_class_value_size (ret_class, NULL);
+		if (id == SN_AsVector || id == SN_AsVector128 || id == SN_AsVector4 || id == SN_AsPlane || id == SN_AsQuaternion) {
+			MonoClass *ret_class = mono_class_from_mono_type_internal (fsig->ret);
+			int ret_size = mono_class_value_size (ret_class, NULL);
 
-		MonoClass *arg_class = mono_class_from_mono_type_internal (fsig->params [0]);
-		int arg_size = mono_class_value_size (arg_class, NULL);
+			MonoClass *arg_class = mono_class_from_mono_type_internal (fsig->params [0]);
+			int arg_size = mono_class_value_size (arg_class, NULL);
 
-		if (arg_size == ret_size)
-			return emit_simd_ins (cfg, klass, OP_XCAST, args [0]->dreg, -1);
+			if (!mini_class_is_simd(cfg, ret_class) || !mini_class_is_simd(cfg, arg_class))
+				return NULL;
 
-		return NULL;
+			if (arg_size != ret_size)
+				return NULL;
+		}
+
+		return emit_simd_ins (cfg, klass, OP_XCAST, args [0]->dreg, -1);
 	}
 	case SN_Ceiling:
 	case SN_Floor: {

[jeffschwMSFT]

can you elaborate on the project that this is for? did the issue occur on iOS/Android/WASM and it also happens to repro on Windows? If so, we can get to the right owning team. However, if this is mono on Windows, we deprecated those packages in .NET 9.

dotnet/docs#41366

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @vitek-karas
See info in area-owners.md if you want to be subscribed.

[saitama951]

@lateralusX sorry for the delay in response, I tried your fix I still see the same assert failure.

Do you happen to know what instruction type it asserts on?
it asserts on a storex_membase

I think when we try to decompose the OP_ARG cases in the xcall we emit the correct dreg there but then the xcall changes to a vcall2 where vcall2 doesn't expect anything in the destination.

/* Represents the decomposed vcall which doesn't return a vtype no more */
MINI_OP(OP_VCALL2,  "vcall2", NONE, NONE, NONE)

so techincally we shouldn't do these decomposition for xcall calls
I tried the following diff

@@ -6759,6 +6908,14 @@ decompose_vtype_opt_store_arg (MonoCompile *cfg, MonoBasicBlock *bb, MonoInst *i
        guint32 *dreg = (guint32*)dreg_int32;
        MonoInst *dest_var = get_vreg_to_inst (cfg, *dreg);
        if (dest_var && dest_var->opcode == OP_ARG && dest_var->klass && mini_class_is_simd (cfg, dest_var->klass)) {
+        switch(ins->opcode)
+        {       
+            case OP_XCALL:
+            case OP_XCALL_MEMBASE:
+            case OP_XCALL_REG:
+                    return;
+            default:
+        }
                MonoInst *varload_ins, *store_ins;
                *dreg = alloc_xreg (cfg);
                NEW_VARLOADA (cfg, varload_ins, dest_var, dest_var->inst_vtype);

This seems to have fixed the error.
@lateralusX do you agree with this?

[saitama951]

can you elaborate on the project that this is for? did the issue occur on iOS/Android/WASM and it also happens to repro on Windows? If so, we can get to the right owning team. However, if this is mono on Windows, we deprecated those packages in .NET 9.

dotnet/docs#41366

oh I didn't know this. This was an experimental effort to get SIMD on s390x which follows a similar convention and similar code path to that of windows. since this assert was reproducible on windows+mono I thought of reporting this.

[lateralusX]

@lateralusX sorry for the delay in response, I tried your fix I still see the same assert failure.

Do you happen to know what instruction type it asserts on?
it asserts on a storex_membase

I think when we try to decompose the OP_ARG cases in the xcall we emit the correct dreg there but then the xcall changes to a vcall2 where vcall2 doesn't expect anything in the destination.

/* Represents the decomposed vcall which doesn't return a vtype no more */
MINI_OP(OP_VCALL2,  "vcall2", NONE, NONE, NONE)

so techincally we shouldn't do these decomposition for xcall calls I tried the following diff

@@ -6759,6 +6908,14 @@ decompose_vtype_opt_store_arg (MonoCompile *cfg, MonoBasicBlock *bb, MonoInst *i
        guint32 *dreg = (guint32*)dreg_int32;
        MonoInst *dest_var = get_vreg_to_inst (cfg, *dreg);
        if (dest_var && dest_var->opcode == OP_ARG && dest_var->klass && mini_class_is_simd (cfg, dest_var->klass)) {
+        switch(ins->opcode)
+        {       
+            case OP_XCALL:
+            case OP_XCALL_MEMBASE:
+            case OP_XCALL_REG:
+                    return;
+            default:
+        }
                MonoInst *varload_ins, *store_ins;
                *dreg = alloc_xreg (cfg);
                NEW_VARLOADA (cfg, varload_ins, dest_var, dest_var->inst_vtype);

This seems to have fixed the error. @lateralusX do you agree with this?

Thanks! Looks like this commit added support for xcalls, breaking the decompose, 9a33ac5. The problem is that the decompose pass removes the return register when switching to vcall and that return register was used by the decompose intrinsic leading to incorrect handling. Not doing this for xcalls make sense and is inline with behavior before the above commit got introduced (it used vcall). Maybe we can exclude the xcall instructions in decompose_vtype_opt_uses_simd_intrinsics instead? I believe we could change that function and drop existing switch (all SIMD operations should set uses_simd_intrinsics) and put the xcall switch first and then check if cfg->uses_simd_instrinsics. You can create a PR with that change if you want and we see how well it stands up against CI.