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

Avoid using a ref local for array element l-value #24070

Merged
merged 12 commits into from
Jan 10, 2018
Merged

Avoid using a ref local for array element l-value #24070

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
30b2023
Avoid using a ref local for array element l-value
cston Jan 5, 2018
823b04e
Update test
cston Jan 5, 2018
f4cb9c2
Avoid ref local for array access only
cston Jan 8, 2018
ec674dc
PR feedback
cston Jan 8, 2018
b533a98
Add test
cston Jan 8, 2018
0b89c5b
Misc.
cston Jan 8, 2018
178af07
Fix unit tests
cston Jan 8, 2018
85118c2
Add non-constant index tests
cston Jan 9, 2018
4c47f27
Fix unit tests (again)
cston Jan 9, 2018
a8aa475
PR feedback
cston Jan 9, 2018
8d15538
PR feedback
cston Jan 9, 2018
f657ba1
Update With block tests
cston Jan 9, 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
48 changes: 27 additions & 21 deletions ...pilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CompoundAssignmentOperator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,29 +507,30 @@ private BoundExpression TransformCompoundAssignmentLHS(BoundExpression originalL
break;

case BoundKind.ArrayAccess:
if (isDynamicAssignment)
Copy link
Member

@VSadov VSadov Jan 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Just removing this seems pretty heavy.

We should do this only when element type is an unsealed reference type.
We definitely do not want to do this with struct arrays or we might degrade fairly common patterns like intArray[i]++

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

Do we have a test that generates / diffs IL to verify the intArray[i]++ case maintains the current code generation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

Copy link
Member

Choose a reason for hiding this comment

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

Since this fix addresses such a corner case, I think the right expectation is that most or none of the existing tests need to change.
And there should be a lot of existing tests doing intArray[i]++ or similar.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like it is the case after the last commit.

{
// In non-dynamic array[index] op= R we emit:
// T& tmp = &array[index];
// *tmp = *L op R;
// where T is the type of L.
//
// If L is an array access, the assignment is dynamic, the compile-time of the array is dynamic[]
// and the runtime type of the array is not object[] (but e.g. string[]) the pointer approach is broken.
// T is Object in such case and we can't take a read-write pointer of type Object& to an array element of non-object type.
//
// In this case we rewrite the assignment as follows:
//
// E t_array = array;
// I t_index = index; (possibly more indices)
// T value = t_array[t_index];
// t_array[t_index] = value op R;

var arrayAccess = (BoundArrayAccess)originalLHS;
var loweredArray = VisitExpression(arrayAccess.Expression);
var loweredIndices = VisitList(arrayAccess.Indices);

return SpillArrayElementAccess(loweredArray, loweredIndices, stores, temps);
if (isDynamicAssignment || !IsInvariantArray(arrayAccess.Expression.Type))
{
// In non-dynamic, invariant array[index] op= R we emit:
// T& tmp = &array[index];
// *tmp = *L op R;
// where T is the type of L.
//
// If L is an array access, the assignment is dynamic, the compile-time of the array is dynamic[]
Copy link
Member

Choose a reason for hiding this comment

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

This paragraph doesn't mention covariant array access. Should it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

// and the runtime type of the array is not object[] (but e.g. string[]) the pointer approach is broken.
// T is Object in such case and we can't take a read-write pointer of type Object& to an array element of non-object type.
//
// In this case we rewrite the assignment as follows:
//
// E t_array = array;
// I t_index = index; (possibly more indices)
// T value = t_array[t_index];
// t_array[t_index] = value op R;
var loweredArray = VisitExpression(arrayAccess.Expression);
var loweredIndices = VisitList(arrayAccess.Indices);

return SpillArrayElementAccess(loweredArray, loweredIndices, stores, temps);
}
}
break;

Expand Down Expand Up @@ -603,6 +604,11 @@ private BoundExpression TransformCompoundAssignmentLHS(BoundExpression originalL
return variableTemp;
}

private static bool IsInvariantArray(TypeSymbol type)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: CLR spec uses "non-variant" term for this.

{
return (type as ArrayTypeSymbol)?.ElementType?.IsSealed == true;
Copy link
Member

Choose a reason for hiding this comment

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

What does this return for T where T : struct?

Copy link
Member

Choose a reason for hiding this comment

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

We can treat T where T : struct arrays as nonvariant.

I think IsSealed returns false for type parameters regardless of constraints, so it may need to be checked separately. Or not at all - it would be a rare and hardly testable scenario.

Copy link
Member Author

@cston cston Jan 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, IsSealed returns false for type parameters (and IsMustInherit returns False in VB), regardless of constraints. I've added tests for Structure and reference type constraints in VB but I'm not able to hit IsInvariantArray with an array of type parameters in C#. I've left the implementations of IsInvariantArray as is, returning false for type parameters.

Copy link
Contributor

@AlekseyTs AlekseyTs Jan 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

? [](start = 57, length = 1)

It feels like the second conditional access is redundant. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

}

private BoundExpression BoxReceiver(BoundExpression rewrittenReceiver, NamedTypeSymbol memberContainingType)
{
return MakeConversionNode(
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenOperators.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4860,7 +4860,7 @@ .locals init (int? V_0)
}

[Fact]
public void TestCompoundOnAfieldOfGeneric()
public void TestCompoundOnAFieldOfGeneric()
{
var source = @"
class Program
Expand Down
153 changes: 153 additions & 0 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 @@ -16073,5 +16073,158 @@ public static explicit operator Test(Span<int> value)

CompileAndVerify(comp, expectedOutput: "SpanOpCalled", verify: Verification.Fails);
}

[Fact]
public void ArrayElementCompoundAssignment_Invariant()
{
string source =
@"class C
{
static void Main()
{
F(new string[] { """" }, ""B"");
}
static void F(string[] a, string s)
{
G(a, s);
System.Console.Write(a[0]);
}
static void G(string[] a, string s)
{
a[0] += s;
}
}";
var verifier = CompileAndVerify(source, expectedOutput: "B");
verifier.VerifyIL("C.G",
@"{
// Code size 17 (0x11)
.maxstack 3
IL_0000: ldarg.0
IL_0001: ldc.i4.0
IL_0002: ldelema ""string""
IL_0007: dup
IL_0008: ldind.ref
IL_0009: ldarg.1
IL_000a: call ""string string.Concat(string, string)""
IL_000f: stind.ref
IL_0010: ret
}");
}

[WorkItem(547533, "https://devdiv.visualstudio.com/DevDiv/_workitems?id=547533")]
[Fact]
public void ArrayElementCompoundAssignment_Covariant()
{
string source =
@"class C
{
static void Main()
{
F(new object[] { """" }, ""A"");
F(new string[] { """" }, ""B"");
}
static void F(object[] a, string s)
{
G(a, s);
System.Console.Write(a[0]);
}
static void G(object[] a, string s)
{
a[0] += s;
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps one more test where the index is a nonconstant expression like a function call - to see two temps.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

}
}";
var verifier = CompileAndVerify(source, expectedOutput: "AB");
verifier.VerifyIL("C.G",
@"{
// Code size 15 (0xf)
.maxstack 4
.locals init (object[] V_0)
IL_0000: ldarg.0
IL_0001: stloc.0
IL_0002: ldloc.0
IL_0003: ldc.i4.0
IL_0004: ldloc.0
IL_0005: ldc.i4.0
IL_0006: ldelem.ref
IL_0007: ldarg.1
IL_0008: call ""string string.Concat(object, object)""
IL_000d: stelem.ref
IL_000e: ret
}");
}

[Fact]
public void ArrayElementCompoundAssignment_ValueType()
{
string source =
@"class C
{
static void Main()
{
F(new int[] { 1 }, 2);
}
static void F(int[] a, int i)
{
G(a, i);
System.Console.Write(a[0]);
}
static void G(int[] a, int i)
{
a[0] += i;
}
}";
var verifier = CompileAndVerify(source, expectedOutput: "3");
verifier.VerifyIL("C.G",
@"{
// Code size 13 (0xd)
.maxstack 3
IL_0000: ldarg.0
IL_0001: ldc.i4.0
IL_0002: ldelema ""int""
IL_0007: dup
IL_0008: ldind.i4
IL_0009: ldarg.1
IL_000a: add
IL_000b: stind.i4
IL_000c: ret
}");
}

[Fact]
public void ArrayElementIncrement_ValueType()
{
string source =
@"class C
{
static void Main()
{
F(new int[] { 1 });
}
static void F(int[] a)
{
G(a);
System.Console.Write(a[0]);
}
static void G(int[] a)
{
a[0]++;
}
}";
var verifier = CompileAndVerify(source, expectedOutput: "2");
verifier.VerifyIL("C.G",
@"{
// Code size 13 (0xd)
.maxstack 3
IL_0000: ldarg.0
IL_0001: ldc.i4.0
IL_0002: ldelema ""int""
IL_0007: dup
IL_0008: ldind.i4
IL_0009: ldc.i4.1
IL_000a: add
IL_000b: stind.i4
IL_000c: ret
}");
}
}
}
20 changes: 13 additions & 7 deletions src/Compilers/VisualBasic/Portable/Lowering/UseTwiceRewriter.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,10 @@ Namespace Microsoft.CodeAnalysis.VisualBasic

Debug.Assert(node.IsLValue)

#If DONT_USE_BYREF_LOCALS_FOR_USE_TWICE Then
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It is not clear why is this conditional compilation got removed from this method. I would expect it to be adjusted to ensure that UseTwiceLValue is still not called when DONT_USE_BYREF_LOCALS_FOR_USE_TWICE is defined. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Added #If.

If IsInvariantArray(node.Expression.Type) Then
Return UseTwiceLValue(containingMember, node, arg)
End If

' Note, as an alternative we could capture reference to the array element in a ByRef temp.
' However, without an introduction of an indirect assignment node, IL-gen is unable to distinguish
' when it should assign indirect or should assign a reference. For now, decided to not introduce
Expand All @@ -232,9 +235,12 @@ Namespace Microsoft.CodeAnalysis.VisualBasic

Debug.Assert(first.IsLValue AndAlso second.IsLValue)
Return New Result(first, second)
#Else
Return UseTwiceLValue(containingMember, node, arg)
#End If

End Function

Private Shared Function IsInvariantArray(type As TypeSymbol) As Boolean
Dim value = TryCast(type, ArrayTypeSymbol)?.ElementType?.IsNotInheritable
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

? [](start = 67, length = 1)

The second conditional access feels redundant. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

Return value.HasValue AndAlso value.GetValueOrDefault()
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

value.HasValue AndAlso [](start = 19, length = 22)

The value.HasValue check is redundant. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

End Function

Private Shared Function UseTwiceLValue(containingMember As Symbol, lvalue As BoundExpression, temporaries As ArrayBuilder(Of SynthesizedLocal)) As Result
Expand Down Expand Up @@ -274,8 +280,8 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
' easier to implement.

Dim receiver As Result = UseTwiceReceiver(containingMember, node.ReceiverOpt, arg)
Dim first = node.Update(receiver.First, fieldSymbol, node.IsLValue, node.ConstantsInProgressOpt, node.Type)
Dim second = node.Update(receiver.Second, fieldSymbol, node.IsLValue, node.ConstantsInProgressOpt, node.Type)
Dim first = node.Update(receiver.First, fieldSymbol, node.IsLValue, suppressVirtualCalls:=False, node.ConstantsInProgressOpt, node.Type)
Dim second = node.Update(receiver.Second, fieldSymbol, node.IsLValue, suppressVirtualCalls:=False, node.ConstantsInProgressOpt, node.Type)

Debug.Assert(first.IsLValue AndAlso second.IsLValue)
Return New Result(first, second)
Expand Down Expand Up @@ -496,7 +502,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
End Sub

#If DONT_USE_BYREF_LOCALS_FOR_USE_TWICE Then
Private Shared Function UseTwiceReceiver(containingMember As Symbol, receiverOpt As BoundExpression, arg As ArrayBuilder(Of TempLocalSymbol)) As Result
Private Shared Function UseTwiceReceiver(containingMember As Symbol, receiverOpt As BoundExpression, arg As ArrayBuilder(Of SynthesizedLocal)) As Result
If receiverOpt Is Nothing Then
Return New Result(Nothing, Nothing)
ElseIf receiverOpt.IsLValue AndAlso receiverOpt.Type.IsReferenceType Then
Expand Down
Loading