Missing normalization in R2R'd codegen for Vector2.get_Item

[kg]

The R2R'd code for Vector2.get_Item believes the high bits of rdx will be zeroes - it contains an int32, the index. The r2r'd code first range checks edx and then happily does a load using rdx (instead of edx) to compute an address. Disassembly:

00007FFC978AB610 48 83 EC 38          sub         rsp,38h  
00007FFC978AB614 F2 0F 10 01          movsd       xmm0,mmword ptr [rcx]  ; rcx is the vector, containing {1,2}
00007FFC978AB618 83 FA 02             cmp         edx,2  ; edx is 0 here
00007FFC978AB61B 73 10                jae         00007FFC978AB62D  
00007FFC978AB61D 0F 29 44 24 20       movaps      xmmword ptr [rsp+20h],xmm0  
00007FFC978AB622 F3 0F 10 44 94 20    movss       xmm0,dword ptr [rsp+rdx*4+20h]  ; we crash here
00007FFC978AB628 48 83 C4 38          add         rsp,38h  
00007FFC978AB62C C3                   ret  
00007FFC978AB62D B9 15 00 00 00       mov         ecx,15h  
00007FFC978AB632 FF 15 B0 CB CD 00    call        qword ptr [7FFC985881E8h]  

Looking at the arguments on the interpreter stack before the call, we can see the high bits of index on the interpreter stack are garbage, so it's natural that rdx has the same garbage in its high bits. I think the interpreter is probably behaving correctly here, but if jitcode expects us to zero the high bits then we probably need to zero them somewhere, maybe in the call stub routines or something.

Call stack:

System_Private_CoreLib!System.Numerics.Vector2.get_Item+0x12
coreclr!CallJittedMethodRetDouble+0x17
coreclr!InvokeCompiledMethod+0x4ed
coreclr!InterpExecMethod+0x8914
coreclr!ExecuteInterpretedMethod+0x209
coreclr!InterpreterStubRetI8+0x12
coreclr!InterpreterStub+0xcc
coreclr!CallDescrWorkerInternal+0x83
coreclr!CallDescrWorkerWithHandler+0x137
coreclr!MethodDescCallSite::CallTargetWorker+0xbd8
coreclr!MethodDescCallSite::Call_RetArgSlot+0x11f
coreclr!RunMainInternal+0x2a9

This appears to be inside TestCallingConvention3Rev in Interpreter.cs

[kg]

@davidwrighton said he has a fix for this so cc'ing him

[dotnet-policy-service]

Tagging subscribers to this area: @BrzVlad, @janvorli, @kg
See info in area-owners.md if you want to be subscribed.

[dotnet-policy-service]

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

[dotnet-policy-service]

Tagging subscribers to this area: @BrzVlad, @janvorli, @kg
See info in area-owners.md if you want to be subscribed.

[davidwrighton]

@kg, I don't have a fix for this, this is actually a JIT bug. We intend to follow the Windows X64 calling convention, which requires that functions accept garbage in the high bits of the general purpose registers if the argument is a 32bit value such as int.

[kg]

Repro steps:

.\build.cmd clr+libs -c Debug -lc Release && .\src\tests\build.cmd Debug -generatelayoutonly && .\src\tests\build.cmd Debug -tree JIT\interpreter
$env:DOTNET_InterpMode=1
.\artifacts\tests\coreclr\windows.x64.Debug\JIT\Interpreter\InterpreterTester\InterpreterTester.cmd

[jakobbotsch]

Yes, JIT even internally can leave the upper bits undefined here. So this is not a disagreement, just a straight up bug.

[jakobbotsch]

In this case GetElement (and some other intrinsics, like WithElement) have an int32 index in IL, but the backend assumes their index is native sized. We either need to do proper normalization in codegen (allocating a temporary register) or ensure they get a cast on top of the index when they are created such that the index is always native typed.
I have preference for the latter since it will allow other normal optimizations to kick in, and it matches how we represent other indexing constructs (like arrays or spans). What do you think @tannergooding?