-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Part 1 of support for C# 12 InlineArray #3467
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.
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
Conversation
@ds5678 this PR covers the basics of InlineArrays... before we can continue with params collections, we will have to add minimal support for collection expressions:
Would you be interested in taking this on? using System;
using System.Runtime.CompilerServices;
public class C {
[InlineArray(4)]
public struct MyStruct
{
private int _;
}
public string ParamsSpanInlineArray() {
return string.Join(", ", "a", "b", "c");
}
public string SpanCollectionInitializer() {
return string.Join(", ", ["a", "b", "c"]);
}
public MyStruct Foo()
{
// return [1, 2, 3, 4]; wouldn't work, because inline arrays are not "collection-expression constructible"
MyStruct array = default;
array[0] = 1;
array[1] = 2;
array[2] = 3;
array[3] = 4;
return array;
}
} |
Sure, I can work on this. |
@@ -435,6 +435,29 @@ public ExpressionWithResolveResult Build(OpCode callOpCode, IMethod method, | |||
argumentList.GetArgumentResolveResults(), isExpandedForm: argumentList.IsExpandedForm, isDelegateInvocation: true)); | |||
} | |||
|
|||
if (settings.InlineArrays && method.DeclaringType.FullName == "<PrivateImplementationDetails>") | |||
{ | |||
var unwrappedTarget = argumentList.Arguments[0].Expression; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argumentList.Arguments
might be empty here.
case "InlineArrayAsReadOnlySpan": | ||
var arrayResolveResult = unwrappedTarget.GetResolveResult(); | ||
unwrappedTarget.RemoveAnnotations<ResolveResult>(); | ||
return unwrappedTarget.Detach().WithRR(new InlineArrayResolveResult(arrayResolveResult, method.ReturnType)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense. A ResolveResult describes the semantics of a C# expression we have generated. If the C# expression doesn't change, it doesn't make sense to swap out the associated ResolveResult?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that our conversion logic needs to consider all possible conversions (for overload resolution), not just the conversion actually picked in the IL program (that wouldn't allow us to detect if we're about to emit C# that would pick the wrong overload).
So InterpolatedStringResolveResult
makes sense (it's associated with a special syntactic form of an argument expression); but InlineArrayResolveResult
doesn't (there's no syntactic difference between an argument expression that is converted to span, and one that is passed as-is without conversion).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should we then represent the implicit conversion from an arbitrary struct
type with the InlineArrayAttribute
to (ReadOnly)Span<T>
?
Given
[InlineArray(4)] struct MyArray { private int elem; }
MyArray GetArray() => default;
int GetIndex() => 0;
int item = GetArray()[GetIndex()];
the last expression is compiled down to (pseudo code, with some extra inlining):
int num = <PrivateImplementationDetails>.InlineArrayAsReadOnlySpan<MyArray, int>(GetArray(), 4)[GetIndex()];
what the transform does is: extract the first argument of InlineArrayAsReadOnlySpan
swap the ResolveResult
in order to account for the implicit conversion to ReadOnlySpan<int>
and replace the call to InlineArrayAsReadOnlySpan
with the first argument.
without the extra ResolveResult, we would get int num = ((ReadOnlySpan<int>)GetArray())[GetIndex()];
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your case is a bit of a red herring: the return value of GetArray()
cannot be converted to ReadOnlySpan<int>
(neither with an explicit cast nor with an implicit conversion), because this conversion only allows lvalues.
We need to treat that particular call combination (conversion to span followed by indexing) as a special case, maybe even introducing a special ILAst instruction for this combination.
For thinking about the conversion modelling, the indexing special case is actively misleading. There's it's more useful to consider Foo(some_array)
, where Foo has multiple overloads (some of which involve the conversion).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In particular for conversion modeling you also you need to think about cases where the IL does not perform the conversion to span, but the C# code might unless we add explicit casts.
For example: in this example the explicit cast to (object)
is required because the overload would otherwise be ambiguous. If our conversion machinery does not consider the span conversion to be possible, OR would think that the cast is not required, and we generate invalid code. Thus our conversion machinery needs to support span conversions in general, not just for a special resolve result.
64e3f36
to
7edda6d
Compare
@dgrunwald please review again... I think this could be merged. Edit: actually I think there is one open question regarding conversions, but that would need some more discussion, because I have no idea where to add it. |
b71ff69
to
15c3c2d
Compare
if (!MatchInlineArrayHelper(targetInst.Method, "InlineArrayAsReadOnlySpan", out var inlineArrayType)) | ||
return false; | ||
|
||
if (targetInst.Arguments is not [var addrInst, LdcI4]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value of the LdcI4 needs to be checked against the length of the inline array type.
} | ||
else | ||
{ | ||
context.Step("call InlineArrayElementRef(addr, index) -> ldelema.inlinearray(addr, index)", inst); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does InlineArrayElementRef
perform bounds-checking? If not, this transform is not semantics-preserving (we'd need to check that index
is a constant and inbounds)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My quick tests show that InlineArrayElementRef is only used for constants where compile-time checking occurs/is possible. Will adjust the transform accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the other case MemoryMarshal.CreateSpan (https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.memorymarshal.createspan?view=net-9.0) is used where the documentation says it does not perform bounds checking either.
I think LdElemaInlineArray can be specified as never performing a bounds check. Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If LdElemaInlineArray is specified as never perform a bounds checks, then the MatchSpanIndexerWithInlineArrayAsSpan
transform would be invalid as it'd be removing the get_Item
bounds check.
Also you wouldn't be able to turn ldelema.inlinearray(addr, index)
into C# array[index]
as the latter does perform a bounds check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah the get_Item call performs the bounds check. Sorry, this slipped my attention. Small mobile phone screen...
be5eefc
to
298c247
Compare
@dgrunwald I just added one new commit, I had to rebase to get the WholeProjectDecompilerTest thread-safety fix. |
No description provided.