-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Delete emit breaks from R2R helper node #114920
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
/ba-g infrastructure timeouts |
MethodDesc targetMethod = (MethodDesc)Target; | ||
if (targetMethod.OwningType.IsInterface) | ||
{ | ||
// Not tested |
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.
We have complete testing on Arm64, but it is not getting hit.
What's the test that hits this path?
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.
@LuckyXu-HF mentioned Methodical_d2
and Methodical_r2
were failing with arm64 release build #114618 (comment).
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.
That comment is tested based on runtime-tag-v9.0.0 ;
I just test this based on the latest merge code in main branch:
commit 2474c6206b1f3d6f18941d05280c08da68231132
Author: Michal Strehovský <[email protected]>
Date: Wed Apr 23 07:31:14 2025 +0200
Delete dead code in the interpreter (#114890)
I think this is now unreachable since the first `if` check would cover this.
And linux-arm64 still hits this path with Methodical_d2
and Methodical_r2
:
$./build.sh clr.aot+libs -c release
$./src/tests/build.sh -priority1 -release -tree:/data/xly/runtime/src/tests/JIT/Methodical/ -nativeaot
Error running test group '/data/xly/runtime/artifacts/tests/coreclr/linux.arm64.Release/JIT/Methodical/Methodical_d2/Methodical_d2.sh' - exit code 1
Error running test group '/data/xly/runtime/artifacts/tests/coreclr/linux.arm64.Release/JIT/Methodical/Methodical_r2/Methodical_r2.sh' - exit code 1
With this PR delete this encoder.EmitINT3();
, then these two passed on linux-arm64 (commit:2474c620)
:
Time [secs] | Total | Passed | Failed | Skipped | Assembly Execution Summary
============================================================================
0.361 | 54 | 51 | 0 | 3 | JIT.Methodical.Methodical_others
0.828 | 207 | 202 | 0 | 5 | JIT.Methodical.Methodical_d2
0.906 | 85 | 84 | 0 | 1 | JIT.Methodical.Methodical_do
0.498 | 210 | 205 | 0 | 5 | JIT.Methodical.Methodical_r2
0.781 | 85 | 84 | 0 | 1 | JIT.Methodical.Methodical_ro
2.070 | 275 | 254 | 0 | 21 | JIT.Methodical.Methodical_d1
1.703 | 278 | 256 | 0 | 22 | JIT.Methodical.Methodical_r1
----------------------------------------------------------------------------
7.147 | 1194 | 1136 | 0 | 58 | (total)
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.
We have complete testing on Arm64, but it is not getting hit.
@jkotas, @MichalStrehovsky, could you point to the pipeline running these to tests? I couldn't find linux arm64 nativeaot tests running Methodical_d2 and Methodical_r2 in the CI (I checked the NAOT outterloop pipelines as well).
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.
It looks like we do not run runtime tests on arm64. @MichalStrehovsky Is that intentional or just an omission?
#114948 is a test-only PR to see how bad it is if enabled and whether there tests are going to fail in the run.
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.
Thanks! Both _r2 and _d2 failed in the CI https://dev.azure.com/dnceng-public/public/_build/results?buildId=1024306&view=logs&j=85c26b1b-9cc8-57fd-8fb3-20f00e5c8018&t=5f4ce5d6-4051-5282-ebd3-cc613bda4144
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.
Yep. I will look into enabling arm64 testing separately.
@LuckyXu-HF found out these fix some R2R tests: #114618 (comment).