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

Jump to bottom

Emit stackalloc block initializer for byte-sized enums #66251

Merged
merged 3 commits into from Jan 10, 2023
Merged

Emit stackalloc block initializer for byte-sized enums #66251

merged 3 commits into from Jan 10, 2023

Conversation

alrz
Copy link
Member

@alrz alrz commented Jan 4, 2023

No description provided.

@alrz alrz requested a review from a team as a code owner January 4, 2023 23:36
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jan 4, 2023
@jcouv jcouv self-assigned this Jan 5, 2023
jcouv
jcouv approved these changes Jan 5, 2023
View reviewed changes
Copy link
Member

@jcouv jcouv left a 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)

@jcouv
Copy link
Member

jcouv commented Jan 5, 2023

@dotnet/roslyn-compiler for second review. Thanks

jcouv
jcouv reviewed Jan 5, 2023
View reviewed changes
src/Compilers/CSharp/Portable/CodeGen/EmitStackAllocInitializer.cs
@@ -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)
Copy link
Member

@jcouv jcouv Jan 5, 2023 •
edited

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 the CreateSpan 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

#61414 doesn't do anything on stackalloc, unlike #57123 but I think that's not merged in.

Copy link
Member

Choose a reason for hiding this comment

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

Right, I mean we can do something like #61414 but for stackalloc. I didn't realize we'd already done a demo branch with exactly that. Thanks for digging up #57123. I'll ping there to see whether we should make a real PR with that change.

Copy link
Member

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.

Copy link
Member

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. The last 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 into main) for tracking purpose.

Copy link
Member

Choose a reason for hiding this comment

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

I opened #69325. Thanks.

@jcouv jcouv merged commit a905e73 into dotnet:main Jan 10, 2023
@ghost ghost added this to the Next milestone Jan 10, 2023
@allisonchou allisonchou mentioned this pull request Jan 11, 2023
Closed
@Cosifne Cosifne modified the milestones: Next, 17.6 P1 Jan 31, 2023
@stephentoub stephentoub mentioned this pull request Aug 2, 2023
Closed
@jjonescz jjonescz mentioned this pull request Dec 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephentoub stephentoub stephentoub left review comments

@jcouv jcouv jcouv approved these changes

@333fred 333fred 333fred approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Milestone
17.6 P1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@alrz @jcouv @333fred @stephentoub @Cosifne