Skip to content

Conversation

@jkoritzinsky
Copy link
Member

These tests take a very long time to build on CI (I've seen upwards of 10 minutes) because they generate millions of lines of test code and then build it, pegging the IO of the machine.

As far as I understand, most of the interesting cases only happen on outerloop runs (primarily in the jitstress-isas- pipelines), which already run all pri0 and pri1 tests. As a result, I'd like to move these tests out to Pri1 to help speed up CI for those of us that don't work on these tests.

We can also add another pipeline that runs the standard CI test modes with just these tests that triggers on a smaller subset of directories if we feel that we want said additional coverage.

These tests take a very long time to build on CI (I've seen upwards of 10 minutes) because they generate millions of lines of test code and then build it, pegging the IO of the machine.

As far as I understand, most of the interesting cases only happen on outerloop runs (primarily in the jitstress-isas- pipelines). As a result, I'd like to move these tests out to Pri1 to help speed up CI for those of us that don't work on these tests.

We can also add another pipeline that runs the standard CI test modes with just these tests that triggers on a smaller subset of directories if we feel that we want said additional coverage.
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Oct 28, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR sets a default test priority for all hardware intrinsics tests by adding a CLRTestPriority property to the directory-level build configuration.

  • Adds CLRTestPriority property set to 1 in the Directory.Build.props file for hardware intrinsics tests

Copy link
Member

@khushal1996 khushal1996 left a comment

Choose a reason for hiding this comment

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

As long as CI runs these tests, I think it is okay to move them to pri1. Because they visibly do take up lot of time during test build.

@jkoritzinsky
Copy link
Member Author

Not nearly enough of a sample size, but this PR took 12 minutes to build the Pri0 tests, whereas #121078 took about 24.75 minutes. This sort of differential makes it much more viable for us to make further changes to improve our test build over time.

@MichalStrehovsky
Copy link
Member

If we're doing this (not entirely opposed), we can also drop JIT/HardwareIntrinsics from here, since it becomes a no-op:

testBuildArgs: 'nativeaot tree ";nativeaot;Loader;Interop;JIT/HardwareIntrinsics;" /p:BuildNativeAotFrameworkObjects=true'

The reason we selectively ran this here is because JIT changes in HW intrinsics regularly broke native AOT outerloops.

My request to the JIT team would be to run both coreclr outerloop and native AOT outerloop on HW intrinsic changes after we stop running them as Pri-0.

Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

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

Seems fine to me. It is indeed much less important now that things have been mostly stable for several years.

Impactful PRs are typically running outerloop and jitstress already.

@JulieLeeMSFT
Copy link
Member

CC @dotnet/jit-contrib.

Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

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

Fine to me, but we need to make sure to trigger them for any SIMD related change in the JIT, SIMD changes are one of the most popular sources of outerloop asserts/bugs

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

Sounds good too me. The number of these tests is actually about 1/2 of all pri1 tests, so I assume this will also save significant runtime time too.

@khushal1996
Copy link
Member

@tannergooding @MichalStrehovsky Is it possible to run all configurations on Pri1 test for these tests as we did for Pri0? I was worried about the coverage we will get on Pri1 tests.

@EgorBo Is there a way we can trigger these tests on CI with a keyword?

… runtime pipeline for NAOT now that they're Pri1
@jkoritzinsky
Copy link
Member Author

jkoritzinsky commented Oct 29, 2025

@khushal1996 the runtime-coreclr outerloop pipeline should cover the same platforms that the runtime pipeline covers (if not more).

@jkoritzinsky jkoritzinsky enabled auto-merge (squash) October 29, 2025 16:15
@AndyAyersMS
Copy link
Member

Our partners like Arm and Intel need to run these tests and now they can't trigger them on their own. Is there some way to fix this?

@jkoritzinsky jkoritzinsky disabled auto-merge October 29, 2025 16:45
@jkoritzinsky
Copy link
Member Author

@AndyAyersMS I can add a pipeline that auto-triggers these tests (and only these ones) on JIT changes. Would that work?

@AndyAyersMS
Copy link
Member

@AndyAyersMS I can add a pipeline that auto-triggers these tests (and only these ones) on JIT changes. Would that work?

Jit changes plus any changes to these tests? Last I knew Arm folks were working on refactoring a bunch of the tests they had added.

@jkoritzinsky
Copy link
Member Author

Sounds good! I'll add the pipeline definition to this PR and ping First Responders to add the pipeline.

…eCLR and NativeAOT when either the tests or the JIT changes.
@jkoritzinsky
Copy link
Member Author

@AndyAyersMS I've added a pipeline definition in 6ce9d04. Let me know if the set of path triggers looks good to you.

@AndyAyersMS
Copy link
Member

@AndyAyersMS I've added a pipeline definition in 6ce9d04. Let me know if the set of path triggers looks good to you.

That looks good to me but I don't typically work on that stuff.

@tannergooding anything else we should include?

@tannergooding
Copy link
Member

That looks reasonable.

We could probably restrict it even more if desired. Really it's if the hwintrinsic* files are touched or one lower/lsra/emit (which aren't touched super frequently otherwise).

We're likely not going to catch anything from just general changes to gentree, morph, etc and the things that are broadly impacting end up running outerloop anyways.

But up to y'all if we want it to be more or less fine grained.

@jkoritzinsky
Copy link
Member Author

I'd rather start coarser and you all can refine it over time.

@jkoritzinsky jkoritzinsky enabled auto-merge (squash) October 29, 2025 21:46
@jkoritzinsky
Copy link
Member Author

/ba-g dev innerloop msbuild timeout unrelated. This change doesn't touch product code or unmanaged code and that leg only touches unmanaged code (and died while building product code, not test code).

@jkoritzinsky jkoritzinsky merged commit d3b4005 into dotnet:main Oct 30, 2025
148 of 156 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants