Implement instantiating and unboxing through portable stublinker code…#106
Merged
davidwrighton merged 2 commits intodotnet:masterfrom Nov 21, 2019
davidwrighton:architecture_neutral_stubs_gen_and_cleanup
Merged
Implement instantiating and unboxing through portable stublinker code…#106davidwrighton merged 2 commits intodotnet:masterfrom davidwrighton:architecture_neutral_stubs_gen_and_cleanup
davidwrighton merged 2 commits intodotnet:masterfrom
davidwrighton:architecture_neutral_stubs_gen_and_cleanup
Conversation
… - Handle only the cases with register to register moves - Followon work to remove the old instantiating/unboxing stub infra (except for x86, x86 will remain special) - Shares abi processing logic with delegate shuffle thunk creation - Architecture specific logic is relatively simple - Do not permit use of HELPERREG in computed instantiating stubs - Fix GetArgLoc such that it works on all architectures and OS combinations Add a JIT stress test case for testing all of the various combinations - Use the same calling convention test architecture that was used as part of tail call work Rename secure delegates to wrapper delegates - Secure delegates are no longer a feature of the runtime - But the wrapper delegate lives on as a workaround for a weird detail of the ARM32 abi
jkotas
reviewed
Nov 19, 2019
| <DefineConstants Condition="'$(FeatureDefaultInterfaces)' == 'true'">$(DefineConstants);FEATURE_DEFAULT_INTERFACES</DefineConstants> | ||
| <DefineConstants Condition="'$(FeatureTypeEquivalence)' == 'true'">$(DefineConstants);FEATURE_TYPEEQUIVALENCE</DefineConstants> | ||
| <DefineConstants Condition="'$(FeatureBasicFreeze)' == 'true'">$(DefineConstants);FEATURE_BASICFREEZE</DefineConstants> | ||
| <DefineConstants Condition="'$(FeaturePortableShuffleThunks)' == 'true'">$(DefineConstants);FEATURE_PORTABLE_SHUFFLE_THUNKS</DefineConstants> |
Member
There was a problem hiding this comment.
Nit: This is not really needed here. FEATURE_PORTABLE_SHUFFLE_THUNKS has no impact on anything implemented in C# and chances that it will ever have are pretty low. There are number of other FEATURE_XXX that are defined for cmake only.
jkotas
reviewed
Nov 19, 2019
| } | ||
|
|
||
| // Otherwise, must be an inner delegate of a SecureDelegate of an open virtual method. In that case, call base implementation | ||
| // Otherwise, must be an inner delegate of a WrapperDelegate of an open virtual method. In that case, call base implementation |
Member
There was a problem hiding this comment.
Suggested change
| // Otherwise, must be an inner delegate of a WrapperDelegate of an open virtual method. In that case, call base implementation | |
| // Otherwise, must be an inner delegate of a wrapper delegate of an open virtual method. In that case, call base implementation |
jkotas
reviewed
Nov 19, 2019
src/coreclr/src/vm/frames.h
Outdated
| } | ||
|
|
||
| virtual Assembly *GetAssembly(); | ||
| virtual Assembly *GetAssembly() |
Member
There was a problem hiding this comment.
Not needed anymore. And the base implementation does not need to be virtual anymore.
jkotas
reviewed
Nov 19, 2019
src/coreclr/src/vm/i386/cgencpu.h
Outdated
| #define ENREGISTERED_RETURNTYPE_INTEGER_MAXSIZE 4 | ||
| #define CALLDESCR_ARGREGS 1 // CallDescrWorker has ArgumentRegister parameter | ||
|
|
||
| #define NO_SHUFFLE_INSTANTIATINGSTUB 1 |
Member
Author
There was a problem hiding this comment.
Ah, no, that's a legacy of an old factoring of the FEATURE flags that was terrible. Let me delete that.
Contributor
|
@davidwrighton is this worth running outerloop testing on? |
Member
Author
|
@jashook I've run outerloop before in the coreclr repo, and it found nothing, so I don't really want to run it more unless I have to. I have a small set of changes from Jan to incorporate, and then I'll run the normal CI again, and then merge the code. |
Member
Author
|
/azp list |
Member
Author
|
/azp run runtime-coreclr |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributor
|
@davidwrighton I have started collecting data on the merged commit. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
Add a JIT stress test case for testing all of the various combinations
Rename secure delegates to wrapper delegates