Skip to content
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

Split the MergedTestWrapper for the HWIntrinsic tests so we can actually filter per platform #84959

Merged
merged 3 commits into from
Apr 18, 2023

Conversation

tannergooding
Copy link
Member

Neither #84948 nor the logic prior to it were actually filtering the tests and keeping them from running on unrelated platforms.

It seems like ClrTestPriority nor CLRTestTargetUnsupported on the main projects impact how the MergedWrapperProjectReference items get run or not.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 18, 2023
@tannergooding
Copy link
Member Author

CC. @trylek, @kunalspathak again

Actually getting tests to run this time so I can confirm that CI doesn't execute them as it seems CI is very different from local testing.

From what I can tell, the MergedWrapperProjectReference effectively ignores any project flags on the referenced projects and so it will include and subsequently run all the tests regardless. x64 jobs for the HardwareIntrinsic_r tests were including the AdvSimd tests, for example. Likewise the Arm jobs were including the Avx tests, despite the ClrTestPriority filter prior to #84948 and also still after the changes in #84948 to use ClrTestTargetUnsupported.

This new change splits it into three distinct groups (Arm, General, and X86) so that the wrapper projects can have the filter instead, which looks to work, but we'll see for certain in CI.

@tannergooding tannergooding marked this pull request as ready for review April 18, 2023 02:32
@tannergooding
Copy link
Member Author

tannergooding commented Apr 18, 2023

-- The original logic I had (two PRs to the file back now), where I conditionally excluded projects from the wrapper (prior to #82221) did work; but since we build on one machine and then copy to a potentially unrelated machine, it meant we might not have tested Arm64 for example.

Ideally the wrapper would respect the priority and/or target support settings of each referenced project merged into it, but since that wasn't working, this seems like the "next best thing".

@tannergooding
Copy link
Member Author

tannergooding commented Apr 18, 2023

It's working as expected in CI: https://dev.azure.com/dnceng-public/public/_build/results?buildId=242445&view=ms.vss-test-web.build-test-results-tab 🎉

Unlike other CI runs, if you filter down to HardwareIntrinsics, AdvSimd, or Avx, you'll see that AdvSimd is only running on Arm64 and Avx is only running on x64 or x86 Windows. -- For an example of a not working CI run, see https://dev.azure.com/dnceng-public/public/_build?definitionId=129&_a=summary&view=runs&branchFilter=30933, which includes examples of before and after #84948 and where you'll see that all tests ran on all platforms without filtering for all 3 runs

@kunalspathak
Copy link
Member

It's working as expected in CI: https://dev.azure.com/dnceng-public/public/_build/results?buildId=242445&view=ms.vss-test-web.build-test-results-tab 🎉

May be I am not seeing things correctly, can you paste a screenshot of the filter you are using to search for AdvSimd as an example?

Here is what I see:

image

@tannergooding
Copy link
Member Author

@kunalspathak, the general filtering doesn't seem to always work and can fail if there are "too many" results from what I've seen.

In the below, see that we have many legs that ran _AdvSimd in prior PRs, including WASM and x64 jobs. While now we only run them on relevant Arm64 legs. The same is true for _Avx and other xarch specific intrinsics on x86/x64.

Prior PRs

image
image

This PR:

image
image

@tannergooding
Copy link
Member Author

tannergooding commented Apr 18, 2023

Notably it looks like we may need 1 further split, pulling Avx512F jobs into their own leg as they still run on OSX despite the filter and that times out in #84909

@kunalspathak
Copy link
Member

It seems like ClrTestPriority nor CLRTestTargetUnsupported on the main projects impact how the MergedWrapperProjectReference items get run or not.

With the new logic, do we still need to keep HWITestsArm64Only and HWITestsXArchOnly properties?

@tannergooding
Copy link
Member Author

With the new logic, do we still need to keep HWITestsArm64Only and HWITestsXArchOnly properties?

Technically no, but it does help with local builds and avoiding building those projects as part of the "standalone" test projects by default.

Copy link
Member

@trylek trylek 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 Tanner for figuring this out!

@tannergooding tannergooding merged commit 327c536 into dotnet:main Apr 18, 2023
@tannergooding tannergooding deleted the hwintrin-test-timeout branch April 18, 2023 14:48
@ghost ghost locked as resolved and limited conversation to collaborators May 18, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants