Runtime-async ref local hoisting

[333fred]

Implements hoisting of ref locals in runtime async. Closes #79763. Commit 1 is a refactoring to extract the direct hoisting logic into a helper type that can be used by runtime async as well, and it has no semantic impact beyond that. Commit 2 actually implements the changes in runtime async, so I recommend reviewing this commit-by-commit for ease of reading. @jcouv @RikkiGibson @dotnet/roslyn-compiler for review.

Relates to test plan #75960

[333fred]

@jakobbotsch @agocke, I'd appreciate if you could look through the expected IL changes here and make sure that they match your expectations.

[jakobbotsch]

I spot checked a few of them and they looked good (and valid). I can do some more validation once we start fuzzing with this new change merged into Roslyn.

[jakobbotsch]

This seems to load array[3] twice, discarding the first value. Unexpected?

[jcouv]

I expect we will load array[3] once before the await (to read the old value) and once after (to assigned the updated value). If we're dropping the first value, then the result should be incorrect. @333fred Could we add verification of the resulting values (in this test or separate)?
Similarly, if we do M()[3] += await ..., I expect we'll have a side-effect spilling the result of M() once, but we should still do the indexing with [3] twice. Do we have test showing side-effects and order of evaluation directly? (this IL is large enough that it's not trivial to tell...)

[jakobbotsch]

The IL here loads the element twice and then also stores the element once. So there's three indexing operations happening: two loads done by the IL right above my comment, before awaiting, and one store done by the IL below, after awaiting.

[jcouv]

Thanks. FWIW, it looks like the state machine IL above also had this problem (IL_003d: pop)

[333fred]

Thanks. FWIW, it looks like the state machine IL above also had this problem (IL_003d: pop)

Thanks for looking at that Julien. Given that, I plan to leave this as is for now, unless it is fairly obvious from a quick investigation.

[jcouv]

I think the extra/discarded read is the sacrificial evaluation that we're discussing elsewhere.
Is the following correct:
When doing an assignment arr[index] = await M();, we cache parts with side-effects, and do a sacrificial evaluation of the hoisted/stripped indexing operation before the await. The index-out-of-bounds exception is thrown before M() is evaluated.

Then I think what we're seeing here is that in a compound assignment arr[index] += await M(); we're doing a sacrificial evaluation and an indexing/read before the await, then an indexing/write after the await.

If that understanding is correct, consider filing an issue to optimize the IL (remove sacrificial evaluation) in compound scenarios.

[333fred]

Filed #80147.

[jcouv]

public class CodeGenAsyncSpillTests : EmitMetadataTestBase

nit: consider adding [CompilerTrait(CompilerFeature.Async)] to CodeGenAsyncSpillTests to make it easier to run all the async-related tests #Closed

---

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncSpillTests.cs:17 in 8c4a58b. [](commit_id = 8c4a58b, deletion_comment = False)

[jcouv]

        wrapper = new BaseMethodWrapperSymbol(containingType, methodBeingWrapped, syntax, methodName);

Not related to this PR: I don't expect we'll need this kind of base method wrapper for runtime-async scenario, but it would be good to confirm and check that we have coverage. For example, BaseAccessInClosure_14_WithILCheck and possibly other tests in that file would be good to cover with runtime-async enabled. #Pending

---

Refers to: src/Compilers/CSharp/Portable/Lowering/MethodToClassRewriter.cs:168 in 8c4a58b. [](commit_id = 8c4a58b, deletion_comment = False)

[jcouv]

    public void SpillCollectionInitializer()

Regarding SpillCollectionInitializer and possibly some other tests: is the plan to enable runtime-async compilation in every test in this file? #Closed

---

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncSpillTests.cs:7295 in 8c4a58b. [](commit_id = 8c4a58b, deletion_comment = False)

[jcouv]

Done with review pass (commit 2). I'll look at the tests tomorrow

[jcouv]

                [F]: Unexpected type on the stack. { Offset = 0x40, Found = Int32, Expected = ref '[System.Runtime]System.Threading.Tasks.Task`1<int32>' }

Not blocking this PR: I failed to notice this in previous PRs, why is the expected type on stack ref Task<int> rather than Task<int>?

---

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncSpillTests.cs:1476 in 8c4a58b. [](commit_id = 8c4a58b, deletion_comment = False)

[333fred]

        var verifier = CompileAndVerify(comp, verify: Verification.Fails with

I updated the tests that have baselines with verification already, but I am not going to update the ones that do not have existing baseline running today in this pr at this point (and this particular test is one such example).

---

In reply to: 3253071615

---

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncSpillTests.cs:1259 in 83e3611. [](commit_id = 83e3611, deletion_comment = False)

[333fred]

        wrapper = new BaseMethodWrapperSymbol(containingType, methodBeingWrapped, syntax, methodName);

I'll add verification to CodeGenAsyncTests.Property21.

---

In reply to: 3252991712

---

Refers to: src/Compilers/CSharp/Portable/Lowering/MethodToClassRewriter.cs:168 in 8c4a58b. [](commit_id = 8c4a58b, deletion_comment = False)

[jcouv]

        // Sacrificial read should ensure that `await` never happens

nit: I'd put that inside Assign (same comment below)

---

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncSpillTests.cs:11915 in 83e3611. [](commit_id = 83e3611, deletion_comment = False)

[jcouv]

LGTM Thanks (commit 4) modulo PEVerify errors hit in CI

[333fred]

@RikkiGibson @dotnet/roslyn for a second review

[RikkiGibson]

Will review tomorrow

[RikkiGibson]

LGTM overall, had various comments and questions though.

[RikkiGibson]

huh, so this is operating on the "S" pushed to stack by IL_0001: ldobj? i.e. we can actually hoist 'this' by-value to the stack, an explicit local is not needed? Neat.

Maybe a similar thing as the other test which eliminated the temp for 'this'.

[jcouv]

LGTM Thanks (commit 7)

[jjonescz]

last commit LGTM

[333fred]

Rebased to make the merge clean; I want to keep these two as separate commits in history for future reference, so squash wouldn't have worked.

[jcouv]

LGTM Thanks (commit 2)