-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
JIT: Handle enums in box-unbox optimization #70167
Conversation
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsCloses #55355 When we box a enum and unbox it as integer (and vice versa) JIT should be able to optimize it as no-op. E.g. https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABABgAJiBGAOgCUBXAOwwEt8YaBhCfAB1YAbGFADKIgG6swMXAG4AsACgy5UQAtsUPgBlsweszYdFS5cQBMlKgHZlAb2Xln5ANoApVhgDiMJiOkACgwATz4YCAAzQIBRJgZ8KgBKJIBdZwB6DPImCAwnF1cAWRgMdQgAEwBJfkFAkrLKmr5BAHk+NggmXBoAQQBzfthcXFYJGCqmQVYmGf60gudiAGZrJHJgCAhBcgAJPAAxQWx+gB4AFQA+QPPyCWxBBhg0clvI4/nF8kclFz/KGzkQKBGYYJKBCDAABWMDAYPujxg5AAZEDQeDITC4Ul3ickuQALwEtEsDHQ2Fg3H9Ux/AC+ynpZhUqz8CXIcQSVG+5F6LwAQuRaUA== New codegen: ; Program.HasFlag
and ecx, edx
xor eax, eax
cmp ecx, edx
sete al
ret
; Total bytes of code: 10 The change is small, the verbosity is due to JIT-EE update, I had to add a flag to
|
Can you please take care of #69900 too since you are changing JIT-EE GUID? |
Done |
@dotnet/jit-contrib PTAL, small change (actual change) |
96704a4
to
8eb0dbc
Compare
src/coreclr/jit/importer.cpp
Outdated
@@ -7455,7 +7455,7 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken, | |||
|
|||
// See if the resolved tokens describe types that are equal. | |||
const TypeCompareState compare = | |||
info.compCompHnd->compareTypesForEquality(unboxResolvedToken.hClass, pResolvedToken->hClass); | |||
info.compCompHnd->compareTypesForEquality(unboxResolvedToken.hClass, pResolvedToken->hClass, false); |
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 wonder if we could just call getTypeForPrimitiveValueClass
on the classes here. I guess it's not the exact same logic but maybe it is ok?
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.
Great idea! addressed 🙂
type1 = type1.IsEnum ? type1.UnderlyingType : type1; | ||
type2 = type2.IsEnum ? type2.UnderlyingType : type2; |
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.
UnderlyingType
returns this
if the type is not an enum.
type1 = type1.IsEnum ? type1.UnderlyingType : type1; | |
type2 = type2.IsEnum ? type2.UnderlyingType : type2; | |
type1 = type1.UnderlyingType; | |
type2 = type2.UnderlyingType; |
8eb0dbc
to
4bf8248
Compare
Closes #55355
When we box a enum and unbox it as integer (and vice versa) JIT should be able to optimize it as no-op. E.g. https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABABgAJiBGAOgCUBXAOwwEt8YaBhCfAB1YAbGFADKIgG6swMXAG4AsACgy5UQAtsUPgBlsweszYdFS5cQBMlKgHZlAb2Xln5ANoApVhgDiMJiOkACgwATz4YCAAzQIBRJgZ8KgBKJIBdZwB6DPImCAwnF1cAWRgMdQgAEwBJfkFAkrLKmr5BAHk+NggmXBoAQQBzfthcXFYJGCqmQVYmGf60gudiAGZrJHJgCAhBcgAJPAAxQWx+gB4AFQA+QPPyCWxBBhg0clvI4/nF8kclFz/KGzkQKBGYYJKBCDAABWMDAYPujxg5AAZEDQeDITC4Ul3ickuQALwEtEsDHQ2Fg3H9Ux/AC+ynpZhUqz8CXIcQSVG+5F6LwAQuRaUA==
New codegen:
The change is small, the verbosity is due to JIT-EE update, I had to add a flag to
compareTypesForEquality
because in some places it's still expected to be more conservative around enums vs integers.