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

Removing IsSIMDVectorAssembly and obsoleting isInSIMDModule#27467

Merged
jkotas merged 7 commits intodotnet:masterfrom
tannergooding:vector-to-corelib
Oct 27, 2019
Merged

Removing IsSIMDVectorAssembly and obsoleting isInSIMDModule#27467
jkotas merged 7 commits intodotnet:masterfrom
tannergooding:vector-to-corelib

Conversation

@tannergooding
Copy link
Member

This resolves https://github.com/dotnet/coreclr/issues/27402.

Now that all of the types from System.Numerics.Vectors.dll have been moved down to corelib, we can remove the logic that was supporting the SIMD intrinsic types in other assemblies.

@tannergooding
Copy link
Member Author

CC. @jkotas, @CarolEidt

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks

bool isSIMDClass(CORINFO_CLASS_HANDLE clsHnd)
{
return info.compCompHnd->isInSIMDModule(clsHnd);
if (isIntrinsicType(clsHnd))
Copy link
Member

Choose a reason for hiding this comment

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

It is unfortunate that isIntrinsicType calls getClassAttribs. getClassAttribs does a lot more. Would it make sense to rename isInSIMDModule on JIT/EE interface to isIntrinsicType ?

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'd be fine with doing so, I'll update after I get back home tonight :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I think this makes the overall logic quite a bit simpler. We were already caching isIntrinsicType in the VM, so this reduces the cost of checking elsewhere.

@tannergooding
Copy link
Member Author

Renamed isInSIMDModule to isIntrinsicType, changed the implementation to returned the cached isIntrinsicType value stored in the method table, updated relevant callsites in the interpreter/compiler to use the new method, and updated the JIT/EE Interface Version.

Packet_IsDelegateCreationAllowed = 155,
Packet_IsFieldStatic = 137, // Added 4/9/2013 - needed for 4.5.1
Packet_IsInSIMDModule = 148, // Added 6/18/2014 - SIMD support
Packet_IsIntrinsicType = 148, // Added 10/26/2019 - SIMD support
Copy link
Member Author

Choose a reason for hiding this comment

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

Does the value tied to the packet name need to differ?


bool isIntrinsicType(CORINFO_CLASS_HANDLE clsHnd)
{
return (info.compCompHnd->getClassAttribs(clsHnd) & CORINFO_FLG_INTRINSIC_TYPE) != 0;
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 grepped for other usages of CORINFO_FLG_INTRINSIC_TYPE and confirmed this was the only usage in the compiler.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Perfect. Thank you!

@tannergooding
Copy link
Member Author

Updated crossgen2, which also defines JITEEVersionIdentifier. The two need to stay in sync and be updated simultaneously.

@jkotas jkotas merged commit ca6c03e into dotnet:master Oct 27, 2019
@CarolEidt
Copy link

Nice cleanup - thanks @tannergooding !

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.

Make isInSIMDModule obsolete

3 participants