Conversation
|
/azp run runtime-interpreter |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Tagging subscribers to this area: @mangod9 |
There was a problem hiding this comment.
Pull Request Overview
This PR adjusts the interpreter mode testing to enable more comprehensive testing with InterpMode=3. The changes modernize the interpreter configuration system and add better support for interpreter compatibility with various system features.
Key changes include:
- Switching from assembly-specific interpreter configuration to a global InterpMode=3 setting
- Adding interpreter stack mark conversion functionality for proper stack walking
- Implementing compatibility adjustments between interpreter mode and hardware intrinsics/ReadyToRun
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/Common/CLRTest.Execute.Batch.targets | Replaces assembly-specific interpreter config with global InterpMode=3 |
| src/tests/Common/CLRTest.Execute.Bash.targets | Replaces assembly-specific interpreter config with global InterpMode=3 |
| src/coreclr/vm/stackwalk.h | Adds function declaration for stack mark conversion |
| src/coreclr/vm/stackwalk.cpp | Implements stack mark conversion for interpreter frames |
| src/coreclr/vm/reflectioninvocation.cpp | Updates SkipStruct to handle interpreter stack marks properly |
| src/coreclr/vm/jitinterface.cpp | Simplifies interpreter loading logic |
| src/coreclr/vm/jithost.cpp | Adds EnableHWIntrinsic configuration support |
| src/coreclr/vm/interpexec.cpp | Improves delegate invocation handling for open virtual dispatch |
| src/coreclr/vm/eeconfig.h | Adds interpreter and hardware intrinsic configuration fields |
| src/coreclr/vm/eeconfig.cpp | Implements interpreter mode configuration logic |
| src/coreclr/vm/codeman.cpp | Adds interpreter mode compatibility with hardware features |
| src/coreclr/vm/appdomain.cpp | Updates CallersDataWithStackMark for interpreter compatibility |
| src/coreclr/interpreter/eeinterp.cpp | Updates interpreter mode documentation comments |
| src/coreclr/interpreter/compiler.cpp | Fixes PlatformNotSupportedException intrinsic handling |
|
|
||
| pFrame = pFrame->PtrNextFrame(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The assertion message should be more descriptive and include context about what stackMark value was being processed to aid debugging.
| LOG((LF_ALWAYS, LL_ALWAYS, "Unable to find InterpMethodContextFrame for stackMark=%p (stackLimit=%p, stackBase=%p)\n", stackMark, pThread->GetCachedStackLimit(), pThread->GetCachedStackBase())); |
| SkipStruct(StackCrawlMark* mark, PTR_Thread thread) : | ||
| pStackMark(mark) | ||
| #ifdef FEATURE_INTERPRETER | ||
| // Since the interpreter has its own stack, we need to get a pointer which can be compared on the real | ||
| // stack so that IsInCalleesFrames can work correctly. | ||
| , stackMarkOnOSStack(ConvertStackMarkToPointerOnOSStack(thread, mark)) | ||
| #endif | ||
| { | ||
| } |
There was a problem hiding this comment.
[nitpick] The constructor initializer list formatting is inconsistent. The comma should be aligned with other initializers, not placed at the beginning of line 701.
| enableHWIntrinsic = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_EnableHWIntrinsic); | ||
| if (CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_InterpMode) >= 3) | ||
| { | ||
| // R2R mode 3 disables all hw intrinsics |
There was a problem hiding this comment.
The comment mentions 'R2R mode 3' but should refer to 'InterpMode 3' for clarity, as this is about interpreter mode, not ReadyToRun mode.
| // R2R mode 3 disables all hw intrinsics | |
| // InterpMode 3 disables all hw intrinsics |
src/coreclr/vm/interpexec.cpp
Outdated
|
|
||
| DELEGATEREF delegateObj = LOCAL_VAR(callArgsOffset, DELEGATEREF); | ||
| DELEGATEREF* delegateObj = LOCAL_VAR_ADDR(callArgsOffset, DELEGATEREF); | ||
| NULL_CHECK(delegateObj); |
There was a problem hiding this comment.
NULL_CHECK is being called on a pointer to DELEGATEREF rather than the DELEGATEREF itself. This should be NULL_CHECK(*delegateObj) or the logic should be restructured to check the actual delegate reference.
| NULL_CHECK(delegateObj); | |
| NULL_CHECK(*delegateObj); |
|
/azp run runtime-interpreter |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| bool allowHWIntrinsic = true; | ||
|
|
||
| #if defined(FEATURE_INTERPRETER) | ||
| if (maxVectorTBitWidth != 128 && CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_InterpMode) >= 2) |
There was a problem hiding this comment.
Can this be GetConfigValue(CLRConfig::EXTERNAL_InterpMode) == 3?
a25a4b8 to
7dad289
Compare
|
/azp run runtime-interpreter |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-interpreter |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…ures in the interpreter - Arguments are up to 16 byte aligned - Handle this in the call stub generator, and call emission. (We already did this in the argument input processing in the interpreter We need to align these at 16byte boundaries as the various ABIs tend to have a hard requirement around 16 byte alignment, when they don't have such a requirement around 32 or 64 byte alignment.
|
/azp run runtime-interpreter |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…n that they do not link to the next block
This reverts commit 004aa94.
… sequences of IL opcodes
- This is needed to ensure that some of our SIMD tests finish in a vaguely reasonable timeframe
- The peeps implmented here make that almost possible, although the behavior in CI hasn't yet been verified
- Peeps implemented
- stloc/ldloc ... This is a minor improvement, but will be needed to handle a future optimization around allowing the il stack to have constant values
- box/unbox.any - Removes unnecessary boxing/unboxing
- typeof(T)==typeof(Y) - This allows us to handle the type testing specialization behavior that is used heavily in the BCL
- typeof(T).IsValueType - Used in the Unsafe.BitCast function
Not yet implemented is giving the interpreter stack a concept of constant values, so that we can optimize brtrue/brfalse and friends into unconditional branches. With this set of changes the if (typeof(T)==typeof(int)) { ... } else if (typeof(T)==typeof(float)) pattern is *much* faster than before, but the not taken paths are still fully generated interpreter code, and there are still many branches.
- Run all the scenarios that do anything with the interpreter - Don't run OSX since that pool seems busy these days, and this is a big run
|
/azp run runtime-interpreter |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run runtime-interpreter |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-interpreter |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-interpreter |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| - linux_arm64 | ||
| - windows_x64 | ||
| - windows_arm64 | ||
| - osx_x64 |
There was a problem hiding this comment.
Why have you removed the osx_arm64? It is actually the most important configuration.
There was a problem hiding this comment.
That was just temporary. I was trying to reduce the load on the servers as I did this big runs to find out if the CI could be coerced into getting good data.
This change disables all coreclr tests that cause the merged coreclr test suites to crash and so preventing us from getting any test pass / fail information from those. This is with running the tests with InterpMode=3 (fully interpreted) This was put together on Windows x64 checked build, we may need to add more for other targets in a separate PR.
|
/azp run runtime-interpreter |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Adjust interpmode to automatically work in a few more ways, so that we can run a larger test run with InterpMode=3