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

Capture stackalloc size expression only if needed #24183

Merged
merged 4 commits into from
Jan 25, 2018
Merged

Capture stackalloc size expression only if needed #24183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fbb5451
Capture stackalloc size expression only if needed
alrz Jan 11, 2018
0179b41
Address test failure
alrz Jan 11, 2018
90f51df
Add additional tests
alrz Jan 12, 2018
59bb3fd
Do not run the StackAllocSpanLengthOverflow test on x64 platform
alrz Jan 25, 2018
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
6 changes: 3 additions & 3 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1290,10 +1290,10 @@ private BoundExpression LowerLiftedBinaryArithmeticOperator(
BoundExpression rightNeverNull = NullableAlwaysHasValue(loweredRight);

BoundExpression boundTempX = leftNeverNull ?? loweredLeft;
boundTempX = CaptureNullableOperandInTempIfNeeded(boundTempX, sideeffects, locals);
boundTempX = CaptureExpressionInTempIfNeeded(boundTempX, sideeffects, locals);

BoundExpression boundTempY = rightNeverNull ?? loweredRight;
boundTempY = CaptureNullableOperandInTempIfNeeded(boundTempY, sideeffects, locals);
boundTempY = CaptureExpressionInTempIfNeeded(boundTempY, sideeffects, locals);

BoundExpression callX_GetValueOrDefault = MakeOptimizedGetValueOrDefault(syntax, boundTempX);
BoundExpression callY_GetValueOrDefault = MakeOptimizedGetValueOrDefault(syntax, boundTempY);
Expand Down Expand Up @@ -1330,7 +1330,7 @@ private BoundExpression LowerLiftedBinaryArithmeticOperator(
type: type);
}

private BoundExpression CaptureNullableOperandInTempIfNeeded(BoundExpression operand, ArrayBuilder<BoundExpression> sideeffects, ArrayBuilder<LocalSymbol> locals)
private BoundExpression CaptureExpressionInTempIfNeeded(BoundExpression operand, ArrayBuilder<BoundExpression> sideeffects, ArrayBuilder<LocalSymbol> locals)
{
if (CanChangeValueBetweenReads(operand))
{
Expand Down
12 changes: 7 additions & 5 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StackAlloc.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Diagnostics;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using System.Collections.Immutable;
using Microsoft.CodeAnalysis.PooledObjects;
using Roslyn.Utilities;
using System.Diagnostics;

namespace Microsoft.CodeAnalysis.CSharp
{
Expand Down Expand Up @@ -35,7 +35,9 @@ public override BoundNode VisitStackAllocArrayCreation(BoundStackAllocArrayCreat
else if (type.OriginalDefinition == _compilation.GetWellKnownType(WellKnownType.System_Span_T))
{
var spanType = (NamedTypeSymbol)stackAllocNode.Type;
var countTemp = _factory.StoreToTemp(rewrittenCount, out BoundAssignmentOperator countTempAssignment);
var sideEffects = ArrayBuilder<BoundExpression>.GetInstance();
var locals = ArrayBuilder<LocalSymbol>.GetInstance();
var countTemp = CaptureExpressionInTempIfNeeded(rewrittenCount, sideEffects, locals);
var stackSize = RewriteStackAllocCountToSize(countTemp, elementType);
stackAllocNode = new BoundConvertedStackAllocExpression(stackAllocNode.Syntax, elementType, stackSize, spanType);

Expand All @@ -44,8 +46,8 @@ public override BoundNode VisitStackAllocArrayCreation(BoundStackAllocArrayCreat

return new BoundSequence(
syntax: stackAllocNode.Syntax,
locals: ImmutableArray.Create(countTemp.LocalSymbol),
sideEffects: ImmutableArray.Create<BoundExpression>(countTempAssignment),
locals: locals.ToImmutableAndFree(),
sideEffects: sideEffects.ToImmutableAndFree(),
value: ctorCall,
type: spanType);
}
Expand Down
115 changes: 98 additions & 17 deletions src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1077,24 +1077,19 @@ public static void Main()

CompileAndVerify(comp, expectedOutput: "10", verify: Verification.Fails).VerifyIL("Test.Main", @"
{
// Code size 29 (0x1d)
// Code size 26 (0x1a)
.maxstack 2
.locals init (System.Span<int> V_0, //x
int V_1)
IL_0000: ldc.i4.s 10
IL_0002: stloc.1
IL_0003: ldloc.1
IL_0004: conv.u
IL_0005: ldc.i4.4
IL_0006: mul.ovf.un
IL_0007: localloc
IL_0009: ldloc.1
IL_000a: newobj ""System.Span<int>..ctor(void*, int)""
IL_000f: stloc.0
IL_0010: ldloca.s V_0
IL_0012: call ""int System.Span<int>.Length.get""
IL_0017: call ""void System.Console.WriteLine(int)""
IL_001c: ret
.locals init (System.Span<int> V_0) //x
IL_0000: ldc.i4.s 40
IL_0002: conv.u
IL_0003: localloc
IL_0005: ldc.i4.s 10
IL_0007: newobj ""System.Span<int>..ctor(void*, int)""
IL_000c: stloc.0
IL_000d: ldloca.s V_0
IL_000f: call ""int System.Span<int>.Length.get""
IL_0014: call ""void System.Console.WriteLine(int)""
IL_0019: ret
}");
}

Expand Down Expand Up @@ -1156,6 +1151,92 @@ .locals init (int V_0, //i
}");
}

[Fact]
public void StackAllocSpanLengthConstantFolding()
{
var comp = CreateCompilationWithMscorlibAndSpan(@"
using System;
class Test
{
public static void Main()
{
const int a = 5, b = 6;
Span<int> x = stackalloc int[a * b];
Console.Write(x.Length);
}
}", TestOptions.ReleaseExe);

CompileAndVerify(comp, expectedOutput: "30", verify: Verification.Fails).VerifyIL("Test.Main", @"
{
// Code size 26 (0x1a)
.maxstack 2
.locals init (System.Span<int> V_0) //x
IL_0000: ldc.i4.s 120
IL_0002: conv.u
IL_0003: localloc
IL_0005: ldc.i4.s 30
IL_0007: newobj ""System.Span<int>..ctor(void*, int)""
IL_000c: stloc.0
IL_000d: ldloca.s V_0
IL_000f: call ""int System.Span<int>.Length.get""
IL_0014: call ""void System.Console.Write(int)""
IL_0019: ret
}");
}

[Fact]
public void StackAllocSpanLengthOverflow()
{
var comp = CreateCompilationWithMscorlibAndSpan(@"
using System;
class Test
{
static void M()
{
Span<int> x = stackalloc int[int.MaxValue];
}

public static void Main()
{
try
{
M();
}
catch (OverflowException)
Copy link
Member

Choose a reason for hiding this comment

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

I think you might not be able to catch this when it actually runs.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, no it is a regular overflow, not a StackOverflow.

Copy link
Member

Choose a reason for hiding this comment

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

Actually this is the problem. On 64bit this is not an overflow since the native int can take this value, but then it becomes SO when we localloc.

I guess on 64bit we should not run the compiled code, only validate the IL.

Copy link
Member Author

@alrz alrz Jan 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

How can I repro locally? Does ReleaseExe.WithPlatform(Platform.X86) help here?

Copy link
Member Author

Choose a reason for hiding this comment

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

or should I skip it for now?

Copy link
Member

Choose a reason for hiding this comment

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

both build and test utilities in the repo root take -test64 switch.

I think build -test64 might do it.

Copy link
Member

Choose a reason for hiding this comment

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

nope :-), just tried build -test64 and it only did a build. Need to run test -test64 after that.

Normally our test failures are not platform specific, so we just rely on CI

Copy link
Member

Choose a reason for hiding this comment

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

I would just make a fix and push the change to the branch.
It should really be something like:

var expectedIL = "....";


if (IntPtr.Size == 4)
{
       CompileAndVerify(comp, expectedOutput: "10", verify: Verification.Fails)
       . . . Verify (expectedIL):...
}
else
{
      // on 64bit the native int does not overflow, so we get StackOverflow instead
      // therefore we will just check the IL
      CompileAndVerify(comp, verify: Verification.Fails, options:TestOptions.ReleaseDll)
       . . . Verify (expectedIL):...
}

{
Console.WriteLine(""overflow"");
}
}
}", TestOptions.ReleaseExe);

var expectedIL = @"
{
// Code size 22 (0x16)
.maxstack 2
IL_0000: ldc.i4 0x7fffffff
IL_0005: conv.u
IL_0006: ldc.i4.4
IL_0007: mul.ovf.un
IL_0008: localloc
IL_000a: ldc.i4 0x7fffffff
IL_000f: newobj ""System.Span<int>..ctor(void*, int)""
IL_0014: pop
IL_0015: ret
}";

var isx86 = (IntPtr.Size == 4);
if (isx86)
{
CompileAndVerify(comp, expectedOutput: "overflow", verify: Verification.Fails).VerifyIL("Test.M", expectedIL);
}
else
{
// On 64bit the native int does not overflow, so we get StackOverflow instead
// therefore we will just check the IL
CompileAndVerify(comp, verify: Verification.Fails).VerifyIL("Test.M", expectedIL);
}
}

[Fact]
public void ImplicitCastOperatorOnStackAllocIsLoweredCorrectly()
{
Expand Down
15 changes: 5 additions & 10 deletions src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15940,16 +15940,11 @@ public static void Main()
{
// Code size 49 (0x31)
.maxstack 2
.locals init (System.Span<int> V_0, //x
int V_1)
IL_0000: ldc.i4.s 33
IL_0002: stloc.1
IL_0003: ldloc.1
IL_0004: conv.u
IL_0005: ldc.i4.4
IL_0006: mul.ovf.un
IL_0007: localloc
IL_0009: ldloc.1
.locals init (System.Span<int> V_0) //x
IL_0000: ldc.i4 0x84
IL_0005: conv.u
IL_0006: localloc
IL_0008: ldc.i4.s 33
IL_000a: newobj ""System.Span<int>..ctor(void*, int)""
IL_000f: stloc.0
IL_0010: ldloca.s V_0
Expand Down