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.

Sign up for GitHub

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

SpillSequenceSpiller.Spill - ensure sequences under ref assignments are spilled. #58657

Merged
merged 2 commits into from
Jan 10, 2022
Merged

SpillSequenceSpiller.Spill - ensure sequences under ref assignments are spilled. #58657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
254835c
SpillSequenceSpiller.Spill - ensure sequences under ref assignments a…
AlekseyTs Jan 6, 2022
b454a6b
Add a test
AlekseyTs Jan 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions src/Compilers/CSharp/Portable/Lowering/SpillSequenceSpiller.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,11 +311,26 @@ private BoundExpression Spill(
continue;

case BoundKind.Sequence:
// neither the side-effects nor the value of the sequence contains await
// (otherwise it would be converted to a SpillSequenceBuilder).
if (refKind != RefKind.None)
{
return expression;
var sequence = (BoundSequence)expression;

PromoteAndAddLocals(builder, sequence.Locals);
builder.AddExpressions(sequence.SideEffects);
expression = sequence.Value;
continue;
}

goto default;

case BoundKind.AssignmentOperator:
var assignment = (BoundAssignmentOperator)expression;
if (assignment.IsRef &&
assignment is not { Left.Kind: BoundKind.Local, Right.Kind: BoundKind.ArrayAccess }) // Optimize for some known to be safe scenarios.
Copy link
Member

@333fred 333fred Jan 6, 2022 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What drives the need to optimize? Do we think this is especially common? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What drives the need to optimize? Do we think this is especially common?

  • Laziness. Don't want to update IL baselines for tests.
  • The IL is actually becomes bigger, less optimal.

{
var left = Spill(builder, assignment.Left, RefKind.Ref);
var right = Spill(builder, assignment.Right, RefKind.Ref);
expression = assignment.Update(left, right, assignment.IsRef, assignment.Type);
}

goto default;
Expand Down
205 changes: 205 additions & 0 deletions src/Compilers/CSharp/Test/Emit/CodeGen/IndexAndRangeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3582,5 +3582,210 @@ class C
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c").WithLocation(10, 5)
);
}

[Fact]
public void PatternIndexArrayAndAwait_01()
{
var src = @"
class C
{
static async System.Threading.Tasks.Task M1(int[] arr)
{
arr[^1] = await System.Threading.Tasks.Task.FromResult(0);
}

static async System.Threading.Tasks.Task Main()
{
var arr = new[] { 123 };
System.Console.WriteLine(arr[0]);
await M1(arr);
System.Console.WriteLine(arr[0]);
}
}
";
CompileAndVerifyWithIndexAndRange(src, expectedOutput:
@"
123
0
").VerifyDiagnostics();
}

[Fact]
public void PatternIndexArrayAndAwait_02()
{
var src = @"
class C
{
static async System.Threading.Tasks.Task M1(int[] arr)
{
(arr[^1], arr[0]) = (123, await System.Threading.Tasks.Task.FromResult(124));
}

static async System.Threading.Tasks.Task Main()
{
var arr = new int[2];
await M1(arr);
System.Console.WriteLine(arr[0]);
System.Console.WriteLine(arr[1]);
}
}
";
CompileAndVerifyWithIndexAndRange(src, expectedOutput:
@"
124
123
").VerifyDiagnostics();
}

[Fact]
public void PatternIndexArrayAndAwait_03()
{
var src = @"
class C
{
static async System.Threading.Tasks.Task M1((int x, int y)[] arr)
{
arr[^1].x = await System.Threading.Tasks.Task.FromResult(124);
}

static async System.Threading.Tasks.Task Main()
{
var arr = new (int x, int y)[1];
await M1(arr);
System.Console.WriteLine(arr[0].x);
}
}
";
CompileAndVerifyWithIndexAndRange(src, expectedOutput:
@"
124
").VerifyDiagnostics();
}

[Fact]
[WorkItem(58569, "https://github.com/dotnet/roslyn/issues/58569")]
public void PatternIndexArrayAndAwait_04()
{
var src = @"
class C
{
static async System.Threading.Tasks.Task M1((int x, int y)[] arr)
{
(arr[^1].x, arr[0].y) = (123, await System.Threading.Tasks.Task.FromResult(124));
}

static async System.Threading.Tasks.Task Main()
{
var arr = new (int x, int y)[2];
await M1(arr);
System.Console.WriteLine(arr[0].y);
System.Console.WriteLine(arr[1].x);
}
}
";
CompileAndVerifyWithIndexAndRange(src, expectedOutput:
@"
124
123
").VerifyDiagnostics();
}

[Fact]
[WorkItem(58569, "https://github.com/dotnet/roslyn/issues/58569")]
public void PatternIndexArrayAndAwait_05()
{
var src = @"
class C
{
static async System.Threading.Tasks.Task M1((int x, int y)[] arr)
{
arr[^1].x += await System.Threading.Tasks.Task.FromResult(124);
}

static async System.Threading.Tasks.Task Main()
{
var arr = new (int x, int y)[] { (1, 2) };
await M1(arr);
System.Console.WriteLine(arr[0].x);
}
}
";
CompileAndVerifyWithIndexAndRange(src, expectedOutput:
@"
125
").VerifyDiagnostics();
}

[Fact]
public void PatternIndexArrayAndAwait_06()
{
var src = @"
class C
{
static async System.Threading.Tasks.Task M1(int[] arr)
{
arr[^1] += await System.Threading.Tasks.Task.FromResult(124);
}

static async System.Threading.Tasks.Task Main()
{
var arr = new int[] { 1 };
await M1(arr);
System.Console.WriteLine(arr[0]);
}
}
";
CompileAndVerifyWithIndexAndRange(src, expectedOutput:
@"
125
").VerifyDiagnostics();
}

[Fact]
public void PatternIndexArrayAndAwait_07()
{
var src = @"
class C
{
static async System.Threading.Tasks.Task M1(int[][] arr)
{
arr[^1][0] += await System.Threading.Tasks.Task.FromResult(124);
}

static async System.Threading.Tasks.Task Main()
{
var arr = new[] { new[] { 1 } };
await M1(arr);
System.Console.WriteLine(arr[0][0]);
}
}
";
CompileAndVerifyWithIndexAndRange(src, expectedOutput:
@"
125
").VerifyDiagnostics();
}
Copy link
Member

@333fred 333fred Jan 6, 2022 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth testing with some slicing as well: Something like arr[1..][^1] to ensure that the slice rewriting bits are working as well. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth testing with some slicing as well: Something like arr[1..][^1] to ensure that the slice rewriting bits are working as well.

I'll add a test. However, a scenario like that isn't interesting for the specific problem I am trying to address in this PR. It doesn't matter how array came to the existence. It is a reference type and it is going to be captured by value. It is the array element (the actual target of the assignment) captured by reference. That is what needs the right spilling.


[Fact]
public void PatternIndexArrayAndAwait_08()
{
var src = @"
class C
{
static async System.Threading.Tasks.Task M1((int x, int y)[] arr)
{
(arr[1..][^1].x, arr[1..][0].y) = (123, await System.Threading.Tasks.Task.FromResult(124));
}

static async System.Threading.Tasks.Task Main()
{
var arr = new (int x, int y)[5];
await M1(arr);
System.Console.WriteLine(""Done"");
}
}
";
CompileAndVerifyWithIndexAndRange(src, expectedOutput: "Done").VerifyDiagnostics();
}
}
}