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

Disable GFNI tests for NAOT #110250

Merged
merged 3 commits into from
Dec 2, 2024
Merged

Conversation

saucecontrol
Copy link
Member

@saucecontrol saucecontrol commented Nov 28, 2024

Fixes #110240

GFNI is enabled opportunistically, and the HWIntrinsics tests check that the methods throw PNSE when not supported. On NAOT, instead of throwing, the instructions are emitted, so they result in illegal instruction instead.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Nov 28, 2024
@saucecontrol
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Commenter does not have sufficient privileges for PR 110250 in repo dotnet/runtime

@saucecontrol
Copy link
Member Author

@MichalStrehovsky
Copy link
Member

/azp run runtime-nativeaot-outerloop

@saucecontrol
Copy link
Member Author

For reference, the test that was failing:

public void RunUnsupportedScenario()
{
TestLibrary.TestFramework.BeginScenario(nameof(RunUnsupportedScenario));
bool succeeded = false;
try
{
RunBasicScenario_UnsafeRead();
}
catch (PlatformNotSupportedException)
{
succeeded = true;
}
if (!succeeded)
{
Succeeded = false;
}
}

This template is common across HWIntrinsics tests, and any of these will fail when an unsupported instruction set is allowed in NAOT.

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas
Copy link
Member

jkotas commented Nov 28, 2024

This template is common across HWIntrinsics tests, and any of these will fail when an unsupported instruction set is allowed in NAOT.

The other tests dealt with the problem by getting disabled: https://github.com/dotnet/runtime/blob/main/src/tests/JIT/HardwareIntrinsics/X86/X86Serialize/Serialize_r.csproj#L6-L7 . Should we do the same here?

Ideally, the opportunistic set would contain all instructions by default. The only reason why it is not the case are JIT instruction encoding limitations.

@saucecontrol saucecontrol changed the title Remove GFNI from NAOT opportunistic support list Disable GFNI tests for NAOT Nov 28, 2024
@saucecontrol
Copy link
Member Author

Got it, thanks. I've only added the property for the base Gfni tests for now. If the outerloop tests are run with --instruction-set=avx or higher, more things can fail. Should we disable NAOT tests for anything that isn't expected to be supported in CI?

@MichalStrehovsky
Copy link
Member

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member

Got it, thanks. I've only added the property for the base Gfni tests for now. If the outerloop tests are run with --instruction-set=avx or higher, more things can fail. Should we disable NAOT tests for anything that isn't expected to be supported in CI?

We don't really test with --instruction-set, except for a couple native AOT smoke tests. We'd need to do more work if we want to support this as a documented switch and that includes running more tests with these switches set (the switch is currently undocumented on learn.microsoft.com and we treat it as soft-documented).

@jkotas
Copy link
Member

jkotas commented Dec 1, 2024

This template is common across HWIntrinsics tests, and any of these will fail when an unsupported instruction set is allowed in NAOT.

It would be nice to fix this template to be compatible with native AOT: #110293

@MichalStrehovsky
Copy link
Member

Thank you!

@tannergooding tannergooding merged commit 342936c into dotnet:main Dec 2, 2024
70 checks passed
@saucecontrol saucecontrol deleted the naot-outerloop-fix branch December 3, 2024 00:56
eduardo-vp pushed a commit to eduardo-vp/runtime that referenced this pull request Dec 5, 2024
* disable GFNI tests for NAOT

* Apply suggestions from code review

---------

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Co-authored-by: Michal Strehovský <MichalStrehovsky@users.noreply.github.com>
mikelle-rogers pushed a commit to mikelle-rogers/runtime that referenced this pull request Dec 10, 2024
* disable GFNI tests for NAOT

* Apply suggestions from code review

---------

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Co-authored-by: Michal Strehovský <MichalStrehovsky@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-crossgen2-coreclr 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.

JIT.HardwareIntrinsics failing on native AOT
4 participants