Skip to content

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 23, 2025
Merged

Delete emit breaks from R2R helper node #114920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0042de3
Delete emit breaks from R2R helper node
am11 Apr 22, 2025
b093d0a
Update RiscV64ReadyToRunHelperNode.cs
am11 Apr 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions ...ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,6 @@ protected override void EmitCode(NodeFactory factory, ref ARM64Emitter encoder,
MethodDesc targetMethod = (MethodDesc)Target;
if (targetMethod.OwningType.IsInterface)
{
// Not tested
Copy link
Member

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?

Copy link
Member Author

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).

Copy link
Contributor

@LuckyXu-HF LuckyXu-HF Apr 23, 2025 •
edited
Loading

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)

Copy link
Member Author

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).

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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.

encoder.EmitINT3();

encoder.EmitMOV(encoder.TargetRegister.Arg1, factory.InterfaceDispatchCell(targetMethod));
encoder.EmitJMP(factory.ExternSymbol("RhpResolveInterfaceMethod"));
}
Expand Down
3 changes: 0 additions & 3 deletions ...mpiler.Compiler/Compiler/DependencyAnalysis/Target_RiscV64/RiscV64ReadyToRunHelperNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,6 @@ protected override void EmitCode(NodeFactory factory, ref RiscV64Emitter encoder
MethodDesc targetMethod = (MethodDesc)Target;
if (targetMethod.OwningType.IsInterface)
{
// Not tested
encoder.EmitBreak();

encoder.EmitMOV(encoder.TargetRegister.Arg1, factory.InterfaceDispatchCell(targetMethod));
encoder.EmitJMP(factory.ExternSymbol("RhpResolveInterfaceMethod"));
}
Expand Down
Loading