-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Convert Array.IsSimpleCopy and CanAssignArray type to managed #104103
Changes from 13 commits
ea09fe2
afd067b
057c04a
0770882
0cd6e54
dd93e95
423c94b
0ef5370
4b6d830
f8b8355
5f8783b
88beba7
f95296f
d48f6f4
4db934b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -818,7 +818,7 @@ public bool CanCompareBitsOrUseFastGetHashCode | |
/// <summary> | ||
/// A type handle, which can wrap either a pointer to a <c>TypeDesc</c> or to a <see cref="MethodTable"/>. | ||
/// </summary> | ||
internal unsafe struct TypeHandle | ||
internal readonly unsafe partial struct TypeHandle | ||
{ | ||
// Subset of src\vm\typehandle.h | ||
|
||
|
@@ -865,6 +865,29 @@ public static TypeHandle TypeHandleOf<T>() | |
{ | ||
return new TypeHandle((void*)RuntimeTypeHandle.ToIntPtr(typeof(T).TypeHandle)); | ||
} | ||
|
||
public static bool AreSameType(TypeHandle left, TypeHandle right) => left.m_asTAddr == right.m_asTAddr; | ||
|
||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
public bool CanCastTo(TypeHandle destTH) | ||
{ | ||
if (m_asTAddr == destTH.m_asTAddr) | ||
return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am wondering whether this logic should live in CastHelpers.cs next to all other casting logic, so that it is not missed if there are any bug fixes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They look quite inconsistent...Methods in CastHelpers are HCalls in jithelpers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. We want to get rid of all FCalls with
Those are slow path for helpers used by the JIT. Fast paths of those helpers are either in assembly code or in C#. The split between JIT helpers and other helpers is blurry. It is not unusual for the two to have overlapping logic. We even have methods that are used as JIT helpers, but they are used for other purposes as well. I do not have strong opinions about the best source file split. Anything we come up with will have some downsides. This can be worked on in a follow up. |
||
|
||
if (!IsTypeDesc && destTH.IsTypeDesc) | ||
return false; | ||
|
||
CastResult result = CastCache.TryGet(CastHelpers.s_table!, (nuint)m_asTAddr, (nuint)destTH.m_asTAddr); | ||
|
||
if (result != CastResult.MaybeCast) | ||
return result == CastResult.CanCast; | ||
|
||
return CanCastTo_NoCacheLookup(m_asTAddr, destTH.m_asTAddr); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: The first thing that the QCall is going to do is repeat the cache lookup... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, the unmanaged It probably does not matter for the one caller added in this PR, but it may matter for future callers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The methods should probably be combined at managed side. They are doing the exact same things. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes (it is fine to do it in a follow up PR). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In fact they aren't. The non-cached cases include nullables, COM and I(Dynamic)Castable interfaces. I don't think specially handling them is worthy, even for future callers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, you would either need to have a bool flag that controls the special handling or introduce two QCalls with similar implementation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you plan to do something about this one? (Unifying with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, together with some other cases sharable between |
||
} | ||
|
||
[LibraryImport(RuntimeHelpers.QCall, EntryPoint = "TypeHandle_CanCastTo")] | ||
[return: MarshalAs(UnmanagedType.Bool)] | ||
private static partial bool CanCastTo_NoCacheLookup(void* fromTypeHnd, void* toTypeHnd); | ||
} | ||
|
||
// Helper structs used for tail calls via helper. | ||
|
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.
If any pointer type goes through the non-simple copy paths, it will result in the same fatal crash.