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.

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

Check opportunistic ISAs in crossgen for CompExactlyDependsOn #90326

Closed
wants to merge 2 commits into from
Closed

Check opportunistic ISAs in crossgen for CompExactlyDependsOn #90326

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f76aa9
Check IsInstructionSetOptimisticallySupported in ShouldCodeNotBeCompi…
EgorBo Aug 10, 2023
08b4c86
Update CorInfoImpl.ReadyToRun.cs
EgorBo Aug 15, 2023
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
7 changes: 4 additions & 3 deletions src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,8 +592,7 @@ public static bool ShouldCodeNotBeCompiledIntoFinalImage(InstructionSetSupport i

if (compExactlyDependsOnList != null && compExactlyDependsOnList.Count > 0)
{
// Default to true, and set to false if at least one of the types is actually supported in the current environment, and none of the
// intrinsic types are in an opportunistic state.
// Default to true, and set to false if at least one of the types is actually (or opportunistically) supported in the current environment.
bool doBypass = true;

foreach (var intrinsicType in compExactlyDependsOnList)
Expand All @@ -604,7 +603,9 @@ public static bool ShouldCodeNotBeCompiledIntoFinalImage(InstructionSetSupport i
// This instruction set isn't supported on the current platform at all.
continue;
}
if (instructionSetSupport.IsInstructionSetSupported(instructionSet) || instructionSetSupport.IsInstructionSetExplicitlyUnsupported(instructionSet))
if (instructionSetSupport.IsInstructionSetSupported(instructionSet) ||
instructionSetSupport.IsInstructionSetOptimisticallySupported(instructionSet) ||
Copy link
Member

Choose a reason for hiding this comment

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

The comment above says "and none of the intrinsic types are in an opportunistic state", so the current behavior looks very intentional and not a bug.

Maybe trigger some optional crossgen2 test runs to see whether the test will identify the problem with this change?

Copy link
Member Author

@EgorBo EgorBo Aug 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

A minimal repro to illustrate the problem:

void Foo()
{
    if (Ssse3.IsSupported)
    {
        Bar();
    }
}

[CompExactlyDependsOn(typeof(Ssse3))]
void Bar()
{
    var a = Ssse3.Shuffle(..);
    ...
}

The current behavior for crossgen2 (exactIsaLevel=Sse2, opportunisticIsaLevel=Sse4.2) is to ignore Bar(). But at the same time it's going to compile Foo as:

void Foo()
{
    if (runtimeSsse3Check == true)
    {
        Bar(); // will trigger Tier0 jit compilation
    }
}

So feels like we do need to prejit methods demanding opportunistic ISAs. Another option is to lift [CompExactlyDependsOn(typeof(Ssse3))] to Sse2 but that won't work - analyzer will complain on missing Ssse3 check and also, analyzer is not aware about our opportunistic ISA level so this has to be done in crossgen instead.

cc @davidwrighton who introduced this attribute and the analyzer in #85481

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I remember we can't use Avx as opportunistic with Sse as exact so presumably we can't hit Vector<> mismatch problems

instructionSetSupport.IsInstructionSetExplicitlyUnsupported(instructionSet))
{
doBypass = false;
}
Expand Down