Skip to content

Conversation

@MichalStrehovsky
Copy link
Member

No description provided.

Copilot AI review requested due to automatic review settings March 18, 2025 09:32
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes the remaining support for universal canonical templates and shared code for generics throughout the type loader and runtime environment. Key changes include the deletion of functions and branches dedicated to universal canonical processing, the removal of related assertions and debug comments, and updates to API calls that now default to using specific canonical forms.

Reviewed Changes

Copilot reviewed 41 out of 41 changed files in this pull request and generated 1 comment.

File Description
src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/TypeDesc.Runtime.cs Removed IsTemplateUniversal and simplified IsTemplateCanonical
src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeBuilderState.cs Removed universal-specific checks when computing native layout info
src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/CanonTypes.Runtime.cs Updated canon type initialization to use the overload without the canonical kind parameter
Various Removed branches and parameters related to universal canonical support in type resolution, mapping tables, and invocation metadata
Comments suppressed due to low confidence (2)

src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/TypeSystemContext.Runtime.cs:190

  • Confirm that removing the UniversalCanonType.RuntimeTypeHandle check does not impact type resolution in any edge cases.
else if (rtth.Equals(UniversalCanonType.RuntimeTypeHandle))

src/coreclr/nativeaot/System.Private.Reflection.Execution/src/Internal/Reflection/Execution/ExecutionEnvironmentImplementation.MappingTables.cs:290

  • Verify that eliminating the explicit canonical form selection in the lookup does not lead to mismatches for methods that previously relied on universal canonical paths.
MethodInvokeInfo methodInvokeInfo = TryGetMethodInvokeInfo(declaringTypeHandle, methodHandle, genericMethodTypeArgumentHandles, methodInfo);

private CanonicalFormKind _canonKind;

public CanonicallyEquivalentEntryLocator(RuntimeTypeHandle typeToFind, CanonicalFormKind kind)
public CanonicallyEquivalentEntryLocator(RuntimeTypeHandle typeToFind)
Copy link

Copilot AI Mar 18, 2025

Choose a reason for hiding this comment

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

Ensure that defaulting to CanonicalFormKind.Specific for all canonical comparisons is intended and thoroughly tested after removing universal canonical support.

Copilot uses AI. Check for mistakes.
}

return _lookupMethodInfo.CanInstantiationsShareCode(_methodInstantiation, _canonFormKind);
return _lookupMethodInfo.CanInstantiationsShareCode(_methodInstantiation, CanonicalFormKind.Specific);
Copy link
Member

Choose a reason for hiding this comment

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

Is deleting CanonicalFormKind left for a future PR? Or do you believe that it is worth it to preserve it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not deleting it from the type system, just in case we want it for ReadyToRun at some point. It is in a leafy location there.

@MichalStrehovsky MichalStrehovsky merged commit 5e7d681 into dotnet:main Mar 18, 2025
94 checks passed
@MichalStrehovsky MichalStrehovsky deleted the typeloaderdeletes branch March 18, 2025 22:49
@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants