Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

ImproveComments - making sure the comment is accurately describing the code #26442

Merged
merged 1 commit into from
Sep 4, 2019

Conversation

cshung
Copy link
Member

@cshung cshung commented Aug 29, 2019

According to my understanding, the code is allowing platform-specific hardware-intrinsics to be compiled if it is in CoreLib by not entering that branch and throws.

@@ -1526,6 +1526,7 @@ MethodTableBuilder::BuildMethodTableThrowing(
{
// Disable AOT compiling for managed implementation of hardware intrinsics.
// We specially treat them here to ensure correct ISA features are set during compilation
// We can allow these to AOT compile in CoreLib since CoreLib versions with the runtime.
Copy link
Member

Choose a reason for hiding this comment

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

CoreLib versioning with the runtime is the reason why have don't throw for Vector64/128/256 use in CoreLib here:

// We can allow these to AOT compile in CoreLib since CoreLib versions with the runtime.
if (!IsNgenPDBCompilationProcess() &&
GetAppDomain()->ToCompilationDomain()->GetTargetModule() != g_pObjectClass->GetModule())
{
COMPlusThrow(kTypeLoadException, IDS_EE_HWINTRINSIC_NGEN_DISALLOWED);
}

It's not the reason in this spot.

I don't recall the reason for this one but you can probably find it in the 100+ comments on #24689 and #24917 if you want. I think it was just a risk-limiting thing because we were late in the cycle.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, I just remembered. We don't allow any intrinsic pregeneration outside CoreLib because the way we do it for CoreLib is that we prevent intrinsic expansion of the IsSupported checks and make that check JITted.

This works great for code that properly guards things with IsSupported. But code that doesn't guard things with IsSupported would get illegal instruction traps instead of PlatformNotSupportedException when the CPU doesn't support it. Those are harder to debug so we limit the risk of customers running into that in their code by just making everything jitted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the explanation, it makes sense. I tried to summarize it as succinctly as I can, can you please take a look again?

@cshung cshung force-pushed the dev/andrewau/improve-comments branch from 1b86217 to dc57a0e Compare September 3, 2019 17:55
Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Thanks for improving the comment!

@cshung cshung merged commit e184d3d into dotnet:master Sep 4, 2019
@cshung cshung deleted the dev/andrewau/improve-comments branch September 4, 2019 14:27
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants