-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Use constant indices when populating Lists and arrays in collection expressions #80999
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
…essions This change modifies the compiler to emit constant indices (ldc.i4.0, ldc.i4.1, etc.) instead of using an incrementing index variable when populating a List<T> via CollectionsMarshal.AsSpan<T> in collection expressions without spread elements. Benefits: - Smaller IL code size - Constant indices may enable better JIT optimizations - Potential for vectorization in some cases When spread elements are present, the index variable is still used as it's needed for the CopyTo optimization. Co-authored-by: CyrusNajmabadi <[email protected]>
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs
Outdated
Show resolved
Hide resolved
…iter_CollectionExpression.cs
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs
Outdated
Show resolved
Hide resolved
…iter_CollectionExpression.cs
|
Note: this leads to a genuine improvement in asm, not just IL. Before, the IL for after, you get: This is a reduction from 9 steps to 6 per element. |
| // When there are no spread elements, we can use constant indices for better codegen. | ||
| // When there are spread elements, we need a mutable index variable for the CopyTo optimization. | ||
| bool useConstantIndices = numberIncludingLastSpread == 0; | ||
| BoundLocal? indexTemp = null; |
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.
review with whitespace off.
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs
Outdated
Show resolved
Hide resolved
|
@copilot please add a test with a couple of fixed elements before a spread. The test should demonstrate that nothing changed and it keeps the existing behavior, namely that we don't use constant indices for anything. In other words, test this case: List<int> y = new();
List<int> x = [1, 2, .. y]; |
Added ListConstruction_FixedElementsBeforeSpread test to verify that when there are fixed elements before a spread element (e.g., [1, 2, ..y]), the compiler uses an index variable for ALL elements instead of constant indices. This confirms the existing behavior is maintained - constant indices are only used when there are no spread elements at all. Co-authored-by: CyrusNajmabadi <[email protected]>
Added test |
| } | ||
| } | ||
|
|
||
| [Fact] |
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.
@copilot add [Fact, WorkItem("...")]
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.
Added WorkItem attribute in commit bd54c95.
Improved the documentation to clarify that indexTemp's null state indicates whether constant compile-time indices or runtime-tracked index is used. Changed the condition from `numberIncludingLastSpread > 0` to `numberIncludingLastSpread != 0` for consistency. Removed unnecessary Debug.Assert calls that are redundant with the null check. Co-authored-by: CyrusNajmabadi <[email protected]>
| // When there are spread elements, we need a mutable index variable for the CopyTo optimization. | ||
| BoundLocal? indexTemp = null; | ||
|
|
||
| if (numberIncludingLastSpread > 0) |
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. Looking at the code from above, I recall now, that this optimization won't ever kick in, until we extend it so that normal elements before the first spread element use constant indices. Preconditions earlier on this code path, make it so the condition on this line will always be true, as currently written.
Sorry for the runaround.
| }, | ||
| tryOptimizeSpreadElement: (ArrayBuilder<BoundExpression> sideEffects, BoundExpression arrayTemp, BoundCollectionExpressionSpreadElement spreadElement, BoundExpression rewrittenSpreadOperand) => | ||
| { | ||
| Debug.Assert(indexTemp is not null); |
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.
@copilot add a comment mentioning that if we have spreads we expect o always need an index currently.
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.
Added comments in commit 38fdece to clarify that when spreads are present, we always need a runtime-tracked index variable.
| }, | ||
| tryOptimizeSpreadElement: (ArrayBuilder<BoundExpression> sideEffects, BoundExpression spanTemp, BoundCollectionExpressionSpreadElement spreadElement, BoundExpression rewrittenSpreadOperand) => | ||
| { | ||
| Debug.Assert(indexTemp is not null); |
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.
@copilot have a similar comment here.
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 comment was already added in commit 38fdece at line 1270 (same location).
…ndex Added comments to clarify that when spread elements are present, we always need a runtime-tracked index variable (indexTemp is not null). Co-authored-by: CyrusNajmabadi <[email protected]>
RikkiGibson
left a comment
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.
Change LGTM. The array element assignment change won't have an effect until later PR when we use const indices for element preceding the first spread, but I'm fine with that
|
|
||
| verifier.VerifyIL("Program.Main", """ | ||
| { | ||
| // Code size 96 (0x60) |
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.
@copilot for all tests that were updated add [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71183")] to them to mark them as being part of that issue.
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.
Added WorkItem attributes to all 5 updated tests in commit 2fab349: SpreadElement_Dynamic_05, ListConstruction_01, ListConstruction_Dynamic_04_StaticBinding, Spread_TargetSpan, and List_AddRange_IEnumerable_ClassConstraint.
Added [WorkItem("#71183")] to all tests
that had their IL expectations updated to use constant indices:
- SpreadElement_Dynamic_05
- ListConstruction_01
- ListConstruction_Dynamic_04_StaticBinding
- Spread_TargetSpan
- List_AddRange_IEnumerable_ClassConstraint
Co-authored-by: CyrusNajmabadi <[email protected]>
Fixes #72568
Fixes #71183
Collection expressions: use constant indices when populating Lists and arrays
This PR implements the optimization requested in #70656 to use constant indices instead of an incrementing variable when populating Lists and arrays in collection expressions.
Changes completed:
CreateAndPopulateListinLocalRewriter_CollectionExpression.csto use constant indices when there are no spread elementsCreateAndPopulateArrayto use constant indices when there are no spread elementsImplementation details:
The optimization applies to both Lists and arrays when:
When spread elements are present (e.g.,
[1, 2, ..y]), the index variable is used for ALL elements, including those before the spread, as it's needed for the CopyTo optimization.The code uses the null state of
indexTempto determine behavior:indexTemp is null: Use constant compile-time indices (0, 1, 2, etc.)indexTemp is not null: Use runtime-tracked index variable (for spread elements)Example improvement:
Before:
After:
Benefits:
Security Summary:
No security vulnerabilities were introduced or discovered by this change.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.