KEMBAR78
Emit stackalloc block initializer for byte-sized enums by alrz · Pull Request #66251 · dotnet/roslyn · GitHub
Skip to content

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 Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jan 4, 2023
@jcouv jcouv self-assigned this Jan 5, 2023
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

}
}
else if (elementType.SpecialType.SizeInBytes() == 1)
else if (elementType.EnumUnderlyingTypeOrSelf().SpecialType.SizeInBytes() == 1)
Copy link
Member

@jcouv jcouv Jan 5, 2023

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.

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

None yet

Development

Successfully merging this pull request may close these issues.

5 participants