Skip to content

Gate emit unit tests on JitDumpEmitUnitTests instead of define#96005

Merged
kunalspathak merged 3 commits intodotnet:mainfrom
a74nh:JitDumpEmitUnitTests_github
Dec 15, 2023
Merged

Gate emit unit tests on JitDumpEmitUnitTests instead of define#96005
kunalspathak merged 3 commits intodotnet:mainfrom
a74nh:JitDumpEmitUnitTests_github

Conversation

@a74nh
Copy link
Contributor

@a74nh a74nh commented Dec 14, 2023

  • Add a common genEmitterUnitTests()
  • Add a config option JitDumpEmitUnitTests instead of using #defines.
  • Add basic string checking to decide which groups to run
  • Add a code to jump over the tests to prevent coreclr from running the tests (which usually causes a segfault loading from bad memory)

@ghost ghost added community-contribution Indicates that the PR has been added by a community member area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Dec 14, 2023
@ghost
Copy link

ghost commented Dec 14, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: a74nh
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@a74nh a74nh force-pushed the JitDumpEmitUnitTests_github branch from aae7f4d to cff8d87 Compare December 14, 2023 10:02
@a74nh
Copy link
Contributor Author

a74nh commented Dec 14, 2023

@kunalspathak @BruceForstall - as suggested on #95918

@a74nh a74nh marked this pull request as ready for review December 14, 2023 11:06
@a74nh a74nh force-pushed the JitDumpEmitUnitTests_github branch 2 times, most recently from 4c1c3da to 2764b13 Compare December 14, 2023 12:05
@a74nh a74nh force-pushed the JitDumpEmitUnitTests_github branch from 2764b13 to 8532427 Compare December 14, 2023 14:14
Copy link
Contributor

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, except it looks like some "garbage" code to be removed.

As a follow-up, I think we should move all the arm64 unit tests to a separate file (named codegenarm64test.cpp?) since they're now over 5000 lines in this 11000 line file.

@a74nh
Copy link
Contributor Author

a74nh commented Dec 15, 2023

As a follow-up, I think we should move all the arm64 unit tests to a separate file (named codegenarm64test.cpp?) since they're now over 5000 lines in this 11000 line file.

Done.

@kunalspathak
Copy link
Contributor

seems like we don't need the altjit check, because we should be able to generate the new sve when running natively on arm64?

if (!compiler->opts.altJit)
{
// No point doing this in a "real" JIT.
return;
}

Copy link
Contributor

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for doing it.

// given set of stress mode names, e.g. STRESS_REGS,
// STRESS_TAILCALL
CONFIG_STRING(JitStressRange, W("JitStressRange")) // Internal Jit stress mode
CONFIG_STRING(JitDumpEmitUnitTests, W("JitDumpEmitUnitTests")) // Dump unit tests from Emit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: will be good to mention example usage like should they be comma-separated, etc. May be in a follow-up PR.

@kunalspathak kunalspathak merged commit a7e738c into dotnet:main Dec 15, 2023
@a74nh
Copy link
Contributor Author

a74nh commented Dec 15, 2023

seems like we don't need the altjit check, because we should be able to generate the new sve when running natively on arm64?

if (!compiler->opts.altJit)
{
// No point doing this in a "real" JIT.
return;
}

I originally had this check in the new common function. But then Arm64 testing would always exit without doing the tests. So, I removed it for all targets because it's gated by the config option anyway.

@a74nh a74nh deleted the JitDumpEmitUnitTests_github branch December 15, 2023 15:32
@github-actions github-actions bot locked and limited conversation to collaborators Jan 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants