This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updating the JIT to support marshaling blittable generics. #23899
Updating the JIT to support marshaling blittable generics. #23899
Changes from all commits
fd0d2a6
7396936
3e4f0e7
c5d6ccc
d074c07
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why is this ifndef necessary? We try to avoid logic differences like this for crossgen.
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.
Vector<T>
is blocked from even being type loaded during crossgen (due to its variable size): https://github.com/dotnet/coreclr/blob/master/src/vm/methodtablebuilder.cpp#L1182There 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.
Ok, make sense. I have not realized that this is to avoid failing here 100% of time during crossgen. Comment may be nice
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.
Is there a matching change to be done for this in crossgen2?
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'm not sure. Looking at crossgen2, I don't see any specific handling of
Vector<T>
and disallowing it as a type.I did find a section for the FieldLayoutAlgorithm: https://github.com/dotnet/coreclr/blob/master/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilerContext.cs#L50
However, that field layout only supports
Vector64/128/256<T>
: https://github.com/dotnet/coreclr/blob/master/src/tools/crossgen2/Common/Compiler/VectorFieldLayoutAlgorithm.csThe only logic I can find for
Vector<T>
is in theSystemVStructClassificator
where it doesn't treat it as an EightBytes struct: https://github.com/dotnet/coreclr/blob/master/src/tools/crossgen2/Common/JitInterface/SystemVStructClassificator.cs#L273There 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.
We may need a change here: https://github.com/dotnet/coreclr/blob/master/src/tools/crossgen2/Common/TypeSystem/Interop/IL/MarshalHelpers.cs#L337
cc @fadimounir
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.
What is the easiest way to test Crossgen2 on an specific test?
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've pushed up a commit which I believe handles this, but it's not been tested (and doesn't look to be covered by CI currently).