- Notifications
You must be signed in to change notification settings - Fork4.1k
Emit stackalloc block initializer for byte-sized enums#66251
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
LGTM Thanks (iteration 1)
@dotnet/roslyn-compiler for second review. Thanks |
| @@ -47,7 +47,7 @@ private void EmitStackAllocInitializers(TypeSymbol type, BoundArrayInitializatio | |||
| EmitElementStackAllocInitializers(elementType, initExprs, includeConstants: false); | |||
| } | |||
| } | |||
| else if (elementType.SpecialType.SizeInBytes() == 1) | |||
| else if (elementType.EnumUnderlyingTypeOrSelf().SpecialType.SizeInBytes() == 1) | |||
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.
Not related to this PR: we could also lift the 1 byte restriction as was done in theCreateSpan PR. Somehow I didn't realize until now.
One second thought, it's less beneficial here. We'd still need to copy the data, so maybe the savings aren't as meaningful.
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.
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.
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.
@jcouv, what became of this? Even just emitting it as a copy from the data section (which the JIT should be able to unroll in many cases) would likely be useful.
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.
@stephentoub It looks like the hackathon project didn't transfer into a committed project/PR. Thelast comment from Jared there is that this just needs keyboard time, but we support it in principle.
We're out of dev bandwidth for .NET 8 (ie. at least for one month) unless urgent. Let's discuss around that time. We could file an 17.9 issue (refresh, resolve conflicts and bring#57123 intomain) for tracking purpose.
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.
I opened#69325. Thanks.
src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenStackAllocInitializerTests.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenStackAllocInitializerTests.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Fred Silberberg <fred@silberberg.xyz>
No description provided.