Eliminate tuple allocations when sequential expressions are involved

[kerams]

[<System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining)>]
let y () = 3

let z () =
    let a, b =
        "".ToString ()
        System.DateTime.Now
        "3".ToString ()
        2, y ()
    System.DateTime.Now
    a + b

becomes

[MethodImpl(MethodImplOptions.NoInlining)]
public static int y()
{
    return 3;
}

public static int z()
{
    "".ToString();
    DateTime now = DateTime.Now;
    "3".ToString();
    int num = y();
    DateTime now2 = DateTime.Now;
    return 2 + num;
}

instead of

[MethodImpl(MethodImplOptions.NoInlining)]
public static int y()
{
    return 3;
}

public static int z()
{
    "".ToString();
    DateTime now = DateTime.Now;
    "3".ToString();
    Tuple<int, int> tuple = new Tuple<int, int>(2, y());
    int item = tuple.Item2;
    int item2 = tuple.Item1;
    DateTime now2 = DateTime.Now;
    return item2 + item;
}

[kerams]

What shape would a test for this take and where would it go?

[kerams]

Ping, I really need this for a follow-up PR :).

[vzarytovskii]

I think IL/CodeGen tests will be sufficient enough. @TIHan @KevinRansom what do you think?

[kerams]

So I've added a test on 3 simple functions that makes sure there are no tuples and the order of operations is preserved. Let me know if more complex tests are required (an example would be nice), otherwise this is ready for review.

[vzarytovskii]

Some infrastructure issue with macOS build, gonna restart.

[kerams]

But net472 and net5 also apparently generate different IL for the System.Console.ReadKey () call.... Let me change that.

[vzarytovskii]

@TIHan or @dsyme can you pls also take a look at it?

[kerams]

@cartermp, sorry for the ping. Just want to make sure this has not been forgotten, even if the PR has been ready for barely more than a week. If you're all simply busy, that's cool.

[TIHan]

Apologies for the wait, I'll take a look at this.

[TIHan]

nit: commented code should be removed
Or explain why the commented code is there.

[kerams]

The comment is there to show the C# representation of the IL below. Thought it would be helpful. Should I still remove it?

[TIHan]

The change looks very straight-forward and very cool.

I would add a few more tests, specifically tests that actually require a tuple to be used.
example:

[<System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining)>]
let y () = 3

type Test =
    [<System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining)>]
    static member test(x: int32 * int32) = x

let z () =
    let a, b =
        "".ToString ()
        System.DateTime.Now
        "3".ToString ()
        Test.test(2, y ())
    System.DateTime.Now
    a + b

[<System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining)>]
let y () = 3

type Test =
    [<System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining)>]
    static member test(x: int32 * int32) = x

let z () =
    let (a, b) as t =
        "".ToString ()
        System.DateTime.Now
        "3".ToString ()
        2, y ()
    System.DateTime.Now
    let _ = Test.test(t)
    a + b

[kerams]

Ok, I've added those two as well as this one, which demonstrates there is no intermediate tuple

let v () =
    let a, b =
        "".ToString () |> ignore
        System.DateTime.Now |> ignore
        "3".ToString () |> ignore
        2L, f ()
    System.DateTime.Now |> ignore
    a, b

The optimization does not kick in in your first example. Since test is not inlined, Test.test(2, y ()) is considered first class tuple use. This is the case even when you remove all sequential expressions sharplab link, so no regression as far as I am concerned.

Please let me know if you want to keep the commented out C# code (see comment above), thanks.

[cartermp]

Thanks for addressing all the feedback!

[kerams]

My pleasure.