KEMBAR78
RuntimeHelpers.CreateSpan optimization for stackalloc by svick · Pull Request #57123 · dotnet/roslyn · GitHub
Skip to content

Conversation

@svick
Copy link
Contributor

@svick svick commented Oct 13, 2021

This PR changes the IL generated for stackalloc in the following way:

  • If an existing optimization applies (loading byte arrays directly from PE data; using initblk when all bytes are the same), there is no change.
  • Otherwise, if it's converted to ReadOnlySpan<T> and all initializers are constant, the whole stackalloc is replaced by a call to CreateSpan.
  • Otherwise (e.g. when converted to Span<T> or when only some initializers are constant), CreateSpan is used as a part of regular stackalloc codegen, together with cpblk.

The IL for array initializers is also changed:

  • If an array initializer for a type larger than byte with all initializers constant is converted to ROS, the whole expression is replaced with CreateSpan.

There are still some questions I have and things that would need addressing to make this code production quality:

  1. Is it okay to call GetPinnableReference (without any pinning) to get the backing data for the result of CreateSpan, for use in cpblk?
  2. Alignment. As I understand it, the compiler currently does not align the metadata blocks containing initialization data in any way. If this is a requirement, that would need to change.
  3. ENC. This optimization does not work with ENC and the code needs to know whether to apply this optimization at the lowering stage. But I don't know how to find out if ENC is used from inside of LocalRewriter. Is there some way to do that?

@ghost ghost added Community The pull request was submitted by a contributor who is not a Microsoft employee. Area-Compilers labels Oct 13, 2021
@svick svick force-pushed the initializespan-helper branch from 972aacf to 0f2120d Compare October 13, 2021 18:13
@svick svick force-pushed the initializespan-helper branch 3 times, most recently from 83489e2 to 609cba8 Compare October 13, 2021 20:35
@svick svick force-pushed the initializespan-helper branch from 609cba8 to 432d5e0 Compare October 13, 2021 22:22
@svick svick force-pushed the initializespan-helper branch from 804a997 to 654aba8 Compare October 14, 2021 13:42
@svick svick marked this pull request as ready for review October 14, 2021 14:57
@svick svick requested a review from a team as a code owner October 14, 2021 14:57
@svick svick force-pushed the initializespan-helper branch from 70dd5b4 to de12627 Compare October 14, 2021 17:31
Copy link
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

cool!

@cston cston merged commit 4f9e976 into dotnet:demos/lowlevelhackathon Oct 14, 2021
@cston
Copy link
Contributor

cston commented Oct 14, 2021

Thanks @svick!

@svick svick deleted the initializespan-helper branch October 14, 2021 19:04
@alrz
Copy link
Member

alrz commented Apr 23, 2022

lowlevelhackathon

So this is not supposed to reach main?

@svick
Copy link
Contributor Author

svick commented Apr 23, 2022

@alrz

So this is not supposed to reach main?

It is, at some point. As far as I can see, the required runtime code is ready (dotnet/runtime#60948 (comment)), so I think the next step would be to get answers to the questions I had above and then this could be probably merged into main.

I don't know if I'm going to be able to do that soon.

@jaredpar
Copy link
Member

It's on the list of features we're trying to get into C# 11. This feature is mostly in the "just need to find some keyboard time" vs. "needs a lot of design". That is why we've pushed it further back in the schedule as we've prioritized items that need more design work.

The schedule is very tight right now though as we had a number of last second asks that we're dealing with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

Development

Successfully merging this pull request may close these issues.

5 participants