Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jit codegen for ARM32 produces incorrect behaviour #45250

Closed
KevinRansom opened this issue Nov 26, 2020 · 7 comments · Fixed by #45527 or #45880
Closed

Jit codegen for ARM32 produces incorrect behaviour #45250

KevinRansom opened this issue Nov 26, 2020 · 7 comments · Fixed by #45527 or #45880
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI regression-from-last-release
Milestone

Comments

@KevinRansom
Copy link
Member

We have an issue with the F# compiler on ARM32: dotnet/fsharp#10454
Title: F# 5.0.100 sdk broken for Linux Arm32 build #10454

I have a much smaller of the issue.

  1. Create a new F# console app:
  2. Replace the template code with:
open System

let u_list_revi f st =
    printfn "u_list_revi - enter"
    let n = 1
    let result = [ for i = 0 to n-1 do yield f st (n-1-i) ]
    printfn "u_list_revi - exit"
    result
let mutable count = 0
let u_tyar_constraint st =
    count <- count + 1
    printfn "u_tyar_constraint: %d" count
    (fun _ -> "A")
    //printfn "u_tyar_constraint - exit"
    //result

let u_tyar_constraints =
    printfn "u_tyar_constraints - entry"
    let result = u_list_revi u_tyar_constraint
    printfn "u_tyar_constraints - exit"
    result

[<EntryPoint>]
let main argv =
    let st = 10

    let a = u_tyar_constraints st
    0 // return an integer exit code

Compile the project and run it on windows:
Observe the output:

u_tyar_constraints - entry
u_tyar_constraints - exit
u_list_revi - enter
u_tyar_constraint: 1
u_list_revi - exit
  1. Copy the generated binaries to an arm device with dotnet sdk for arm32 installed
    run the binary
dotnet FuncArmRepro.dll

observe the output:

pi@kevinr-pi:~/repos/Repro $ dotnet FuncArmRepro.dll
u_tyar_constraints - entry
u_tyar_constraints - exit
u_list_revi - enter
u_tyar_constraint: 1
u_tyar_constraint: 2
u_list_revi - exit
pi@kevinr-pi:~/repos/Repro $ 

Note that u_tyar_constraint: is invoked twice on the arm and once on Windows.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels Nov 26, 2020
@am11
Copy link
Member

am11 commented Nov 26, 2020

Without the arm32 device, we can also use Debian arm32 docker:

$ dotnet build && dotnet build -c Release
$ tar czf repro.tar.gz bin/

$ docker run mcr.microsoft.com/dotnet-buildtools/prereqs:debian-9-arm32v7-20200918130550-bfcd90a
$ docker cp repro.tar.gz $(docker ps --format '{{.ID}}' | head -1):/
$ docker exec $(docker ps --format '{{.ID}}' | head -1) sh

# inside docker shell
$ mkdir myrepro
$ tar xzf repro.tar.gz -C myrepro

$ mkdir ~/.dotnet
$ curl -sSL https://aka.ms/dotnet/net5/5.0.1xx/daily/Sdk/dotnet-sdk-linux-arm.tar.gz | tar xzf - -C ~/.dotnet

$ ~/.dotnet/dotnet    myrepro/bin/Release/net5.0/repro.dll
u_tyar_constraints - entry
u_tyar_constraints - exit
u_list_revi - enter
u_tyar_constraint: 1
u_tyar_constraint: 2
u_list_revi - exit

(same result with myrepro/bin/Debug/net5.0/repro.dll)

@JulieLeeMSFT JulieLeeMSFT added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration arch-arm32 and removed untriaged New issue has not been triaged by the area owner labels Nov 30, 2020
@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Nov 30, 2020
@JulieLeeMSFT JulieLeeMSFT added the os-linux Linux OS (any supported distro) label Nov 30, 2020
@echesakov
Copy link
Contributor

Investigating.

@echesakov
Copy link
Contributor

echesakov commented Dec 2, 2020

I suspected that the issue was some sort of weird interaction between tail call via helper and generics, so I talked to @erozenfeld who was working in this area in the past.

He suggested disabling fast tail call optimization on x64 (we don't have the optimization on arm32) and see if the issue reproduces on that platform.

It does and that should make the debugging quite easier:

F:\echesako\Runtime_45250>set COMPlus_FastTailCalls=0
F:\echesako\Runtime_45250>F:\echesako\git\runtime3\artifacts\tests\coreclr\Windows_NT.x64.Checked\Tests\Core_Root\CoreRun.exe bin\Release\netcoreapp5.0\Runtime_45250.dll
u_tyar_constraints - entry
u_tyar_constraints - exit
u_list_revi - enter
u_tyar_constraint: 1
u_tyar_constraint: 2
u_list_revi - exit

@echesakov echesakov removed arch-arm32 os-linux Linux OS (any supported distro) labels Dec 2, 2020
@echesakov
Copy link
Contributor

echesakov commented Dec 3, 2020

I constructed a simpler repro that does not require F# in #45527. Presumably the test should fail on any platform that supports tail call via helpers (except x86).

The issue origin is how the JIT transforms tail. callvirt to a generic virtual method that has as its this argument another call as shown in an example below:

 ldarg.0
 callvirt   instance class Runtime_45250.Program/Func`1<!!0> Runtime_45250.Program/FuncGetter::Get<string>()
 tail.
 callvirt   instance void class Runtime_45250.Program/Func`1<string>::Run()
 ret

The IL snippet corresponds to the following C# code (with small modification of adding tail. prefix):

static void Run(FuncGetter funcGetter)
{
    funcGetter.Get<string>().Run();
}

In fgMorphTailCallViaHelpers such tree:

fgMorphTailCallViaHelpers (before):
               [000006] --CXG-------              *  CALLV ind void   Func`1[__Canon][System.__Canon].Run
               [000004] --C-G------- this in rcx  \--*  CALL nullcheck ref    FuncGetter.Get
               [000003] ------------ this in rcx     +--*  LCL_VAR   ref    V00 arg0
               [000005] H----------- arg1            \--*  CNS_INT(h) long   0x7ff7e1842558 method

would be transformed into a call to dispatcher and an IL stub:

fgMorphTailCallViaHelpers (after):
               [000019] --CXG+------              *  COMMA     void
               [000006] --CXG+------              +--*  CALL      void   ILStubClass.IL_STUB_StoreTailCallArgs
               [000030] -ACXG-----L- arg0 SETUP   |  +--*  ASG       ref
               [000029] D------N----              |  |  +--*  LCL_VAR   ref    V05 tmp4
               [000004] --CXG+------              |  |  \--*  CALL nullcheck ref    FuncGetter.Get
               [000003] -----+------ this in rcx  |  |     +--*  LCL_VAR   ref    V00 arg0
               [000005] H----+------ arg1 in rdx  |  |     \--*  CNS_INT(h) long   0x7ff7e1842558 method
               [000033] -ACXG-----L- arg1 SETUP   |  +--*  ASG       long
               [000032] D------N----              |  |  +--*  LCL_VAR   long   V06 tmp5
               [000018] --CXG+------              |  |  \--*  CALL help long   HELPER.CORINFO_HELP_VIRTUAL_FUNC_PTR
               [000025] -ACXG-----L- arg0 SETUP   |  |     +--*  ASG       ref
               [000024] D------N----              |  |     |  +--*  LCL_VAR   ref    V04 tmp3
               [000013] --CXG+------              |  |     |  \--*  CALL      ref    FuncGetter.Get
               [000014] -----+------ this in rcx  |  |     |     +--*  LCL_VAR   ref    V00 arg0
               [000015] H----+------ arg1 in rdx  |  |     |     \--*  CNS_INT(h) long   0x7ff7e1842558 method
               [000026] ------------ arg0 in rcx  |  |     +--*  LCL_VAR   ref    V04 tmp3
               [000016] H----+------ arg1 in rdx  |  |     +--*  CNS_INT(h) long   0x7ff7e1842960 token
               [000017] H----+------ arg2 in r8   |  |     \--*  CNS_INT(h) long   0x7ff7e18427d0 token
               [000031] ------------ arg0 in rcx  |  +--*  LCL_VAR   ref    V05 tmp4
               [000034] ------------ arg1 in rdx  |  \--*  LCL_VAR   long   V06 tmp5
               [000008] --CXG+------              \--*  CALL      void   System.Runtime.CompilerServices.RuntimeHelpers.DispatchTailCalls
               [000012] -----+------ arg0 in rcx     +--*  ADDR      long
               [000011] ----G+-N----                 |  \--*  LCL_VAR   long  (AX) V03 ReturnAddress
               [000010] H----+------ arg1 in rdx     +--*  CNS_INT(h) long   0x7ff7e1508410 ftn
               [000009] -----+------ arg2 in r8      \--*  CNS_INT   long   0

A target of such tail call would be transformed to a helper call CORINFO_HELP_VIRTUAL_FUNC_PTR. During that transformation a tree corresponding to thisArg would be cloned and, as a consequence, evaluated twice. What that means, method Get would be called twice.

The same happened in the reported F# program where Microsoft.FSharp.Core.FSharpFunc:InvokeFast was transformed similarly and u_tyar_constraint was called twice.

The fix to the issue should be spilling thisArg once before using it later during CORINFO_HELP_VIRTUAL_FUNC_PTR call and during IL_STUB_StoreTailCallArgs call.

Working on the fix.

cc @dotnet/jit-contrib

@echesakov echesakov removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Dec 3, 2020
echesakov added a commit to echesakov/runtime that referenced this issue Dec 9, 2020
echesakov added a commit to echesakov/runtime that referenced this issue Dec 10, 2020
echesakov added a commit that referenced this issue Dec 10, 2020
* Add regression test for #45250

* Add VirtCallThisHasSideEffects to more_tailcalls.cs and update more_tailcalls.il

* Spill "this" if needed to avoid evaluating it twice when "this" is used to compute the target function pointer in morph.cpp
@echesakov
Copy link
Contributor

Reopen and change milestone to 5.0 to track backporting of #45527

@KevinRansom
Copy link
Member Author

Nice ... thanks

Anipik pushed a commit that referenced this issue Dec 11, 2020
…per call (#45880)

* Add regression test for #45250

* Add VirtCallThisHasSideEffects to more_tailcalls.cs and update more_tailcalls.il

* Spill "this" if needed to avoid evaluating it twice when "this" is used to compute the target function pointer in morph.cpp
@echesakov
Copy link
Contributor

Closed with #45880 in 5.0

@ghost ghost locked as resolved and limited conversation to collaborators Jan 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI regression-from-last-release
Projects
Archived in project
5 participants