Intrinsics analyzer and fixes #85481

davidwrighton · 2023-04-27T19:11:44Z

As a follow up to #85275, implement an analyzer and an attribute the Crossgen2 will respect which will make it difficult to encounter unexpected behavior at runtime due to quirks of how we compile System.Private.CoreLib.dll with Crossgen2.

See the change to vectors-and-intrinsics.md for details of what the new system will check.

NOTE: I believe that functionally this change is complete. I need to check crossgen2 performance, and the name of the new attribute is terrible. I'm looking for better names.

This analyzer detects the use of all platform intrinsics and checks to ensure that they are all used either protected by an if statement OR ternary operator which checks an appropriate IsSupported flag, or that the intrinsic is used within a method where the behavior of platform support for the intrinsic is not allowed to vary between compile time and runtime. The supported conditional checks are 1. Simple if statement checking IsSupported flag surrounding usage ``` if (PlatformIntrinsicType.IsSupported) { PlatformIntrinsicType.IntrinsicMethod(); } ``` 2. If statement check checking a platform intrinsic type which implies that the intrinsic used is supported. ``` if (Avx2.X64.IsSupported) { Avx2.IntrinsicMethod(); } ``` 3. Nested if statement where there is an outer condition which is an OR'd together series of IsSupported checks for mutually exclusive conditions and where the inner check is an else clause where some checks are excluded from applying. ``` if (Avx2.IsSupported || ArmBase.IsSupported) { if (Avx2.IsSupported) { // Do something } else { ArmBase.IntrinsicMethod(); } } ``` In addtion, this change adds a new attribute, currently named System.Runtime.BypassReadyToRunForIntrinsicsHelperUse which can be used to identify methods which are called expecting to use some particular set of platform intrinsics. These functions do not need to have explicit checks, and while this is not yet implemented in the change, the attribute will change the behavior of crossgen2 so that code will only be generated if the platform intrinsic support is well known to be either enabled or disabled at runtime. In addition, the method will only be compiled or be inlineable if the platform intrinsic support is enabled that matches at least one of the attributes applied to that method.

… intrinsics usages

…pect the new attribute

jkoritzinsky

Can we add unit tests for this just to make it easier to iterate on in the future? Since it's in the libraries test tree, it should be pretty easy to add them.

src/libraries/System.Private.CoreLib/gen/IntrinsicsInSystemPrivateCoreLibAnalyzer.cs

jkoritzinsky · 2023-04-27T22:37:51Z

src/libraries/System.Private.CoreLib/gen/IntrinsicsInSystemPrivateCoreLibAnalyzer.cs

+        // You can change these strings in the Resources.resx file. If you do not want your analyzer to be localize-able, you can use regular strings for Title and MessageFormat.
+        // See https://github.com/dotnet/roslyn/blob/main/docs/analyzers/Localizing%20Analyzers.md for more on localization


Suggested change

// You can change these strings in the Resources.resx file. If you do not want your analyzer to be localize-able, you can use regular strings for Title and MessageFormat.

// See https://github.com/dotnet/roslyn/blob/main/docs/analyzers/Localizing%20Analyzers.md for more on localization

src/libraries/System.Private.CoreLib/gen/IntrinsicsInSystemPrivateCoreLibAnalyzer.cs

docs/design/coreclr/botr/vectors-and-intrinsics.md

sbomer · 2023-04-28T14:17:13Z

docs/design/coreclr/botr/vectors-and-intrinsics.md

-
-ACCEPTABLE Code
-```csharp
-using System.Runtime.Intrinsics.X86;


I found this example helpful; could we keep it in the crossgen2 section?

The issue is that the example is no longer correct. With the new attribute, the "Unacceptable" code is now legal to use, and the analyzer will ensure that you put the attribute on it.

The unacceptable code is still illegal without an attribute if I understand correctly - I found it helpful in understanding the problem that the attribute solves.

sbomer · 2023-04-28T14:30:32Z

src/coreclr/tools/Common/JitInterface/ThunkGenerator/InstructionSetGenerator.cs

+                    InstructionSetInfo vlInstructionSet = null;
+                    foreach (var potentialVLinstructionSet in _instructionSets)
+                    {
+                        if (instructionSet.Architecture != architecture) continue;


Suggested change

if (instructionSet.Architecture != architecture) continue;

if (potentialVLinstructionSet.Architecture != architecture) continue;

This check still looks redundant with the check above.

src/coreclr/tools/Common/JitInterface/ThunkGenerator/InstructionSetGenerator.cs

src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs

sbomer · 2023-04-28T14:58:37Z

src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunXmlRootProvider.cs

                try
                {
-                    if (!CorInfoImpl.ShouldSkipCompilation(method))
+                    if (!CorInfoImpl.ShouldSkipCompilation(null, method))


Why don't we pass the InstructionSetSupport everywhere this is called?

src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Packed.cs

...ibraries/System.Private.CoreLib/src/System/Runtime/BypassReadyToRunForIntrinsicsHelperUse.cs

MihaZupan · 2023-04-28T20:47:24Z

src/libraries/System.Private.CoreLib/src/System/IndexOfAnyValues/IndexOfAnyAsciiSearcher.cs

+        [System.Runtime.BypassReadyToRunForIntrinsicsHelperUse(typeof(Sse))]
+        [System.Runtime.BypassReadyToRunForIntrinsicsHelperUse(typeof(AdvSimd))]
+        [System.Runtime.BypassReadyToRunForIntrinsicsHelperUse(typeof(PackedSimd))]


Nit:

Suggested change

[System.Runtime.BypassReadyToRunForIntrinsicsHelperUse(typeof(Sse))]

[System.Runtime.BypassReadyToRunForIntrinsicsHelperUse(typeof(AdvSimd))]

[System.Runtime.BypassReadyToRunForIntrinsicsHelperUse(typeof(PackedSimd))]

[BypassReadyToRunForIntrinsicsHelperUse(typeof(Sse))]

[BypassReadyToRunForIntrinsicsHelperUse(typeof(AdvSimd))]

[BypassReadyToRunForIntrinsicsHelperUse(typeof(PackedSimd))]

Should it be Sse2 instead of Sse here?

Yes. This is actually showing one of the limitations of the analyzer, which is that if you follow the pattern of having a helper which uses multiple variants which are explicitly checked, it isn't good at requiring the exact right attribute.

I should be able to make the analyzer detect this situation and issue a slightly different diagnostic, which could be suppressed with a pragma warning disable for legit cases.

….Private.CoreLib

- These use a peculiar construct but its legal as its all containing in 1 method. If it got more complex than that, the analysis would be quite a bit harder

- Handle intrinsics calls from nested intrinsics types - Handle IsSupported checks in the presence of an attribute which explicitly allows an IsSupported check as well as one which doesn't, but is related - Add suppressions for Avx2 IsSupported checks in IndexOfAnyAsciiSearcher helper functions

…zerAndFixes

…don't pass a null InstructionSetSupport into the ShouldSkipCompilation logic, just feed it around everywhere.

davidwrighton · 2023-05-10T23:09:57Z

@MihaZupan @sbomer @jkoritzinsky all of you had good feedback. I believe I've addressed it all, and things are working again. Could you please take another look at this?

sbomer

A couple more nits, otherwise LGTM. Thanks!

src/libraries/System.Private.CoreLib/gen/IntrinsicsInSystemPrivateCoreLibAnalyzer.cs

…zerAndFixes

…of them. Also we needed some new pragma warning disables to make the new logic work

MihaZupan

Just questions around where the annotations are needed

src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Vector128.cs

MihaZupan · 2023-05-15T16:49:01Z

src/libraries/System.Private.CoreLib/src/System/SearchValues/IndexOfAnyAsciiSearcher.cs

+        [CompExactlyDependsOn(typeof(Sse2))]
+        [CompExactlyDependsOn(typeof(AdvSimd))]
+        [CompExactlyDependsOn(typeof(PackedSimd))]


Are these needed here given that the method doesn't directly use any of them?

Yes. Direct use is not particular important. Its transitive use which is most important. Effectively, we need to a be aware of all possible inlining behaviors.

Effectively, we need to a be aware of all possible inlining behaviors.

Thanks, this part wasn't obvious to me (and pretty much answers all the other questions as well).

src/libraries/System.Private.CoreLib/src/System/SearchValues/IndexOfAnyAsciiSearcher.cs

src/libraries/System.Private.CoreLib/src/System/SearchValues/ProbabilisticMap.cs

src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Packed.cs

MihaZupan · 2023-05-15T16:56:03Z

src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Packed.cs

                    Vector128<byte> packedValue0 = Vector128.Create((byte)value0);
                    Vector128<byte> packedValue1 = Vector128.Create((byte)value1);

+#pragma warning disable IntrinsicsInSystemPrivateCoreLibConditionParsing // A negated IsSupported condition isn't parseable by the intrinsics analyzer, but in this case, it is only used in combination


Nit: can this comment be moved before the pragmas, it's hard to read the way it's indented currently.

Feel free to refactor after I check in. This comment style has made it much easier to get the correct pragmas into place as I look at lots of code, but that need will evaporate once everything is in.

src/libraries/System.Private.CoreLib/src/System/Text/Unicode/Utf16Utility.Validation.cs

src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilerContext.cs

src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs

jkoritzinsky · 2023-05-15T17:38:30Z

...oreLib/tests/IntrinsicsInSystemPrivatecoreLibAnalyzer.Tests/CSharpAnalyzerVerifier`1+Test.cs

+                SolutionTransforms.Add((solution, projectId) =>
+                {
+                    var compilationOptions = solution.GetProject(projectId).CompilationOptions;
+                    solution = solution.WithProjectCompilationOptions(projectId, compilationOptions);
+
+                    return solution;
+                });


Does this solution transform do anything? It looks like a no-op.

In any case, there's a CreateCompilationOptions method that you can override to customize the compilation options instead of using a solution transform.

Honestly, I have no idea. It came from the default make an analyzer template in VS, and doesn't seem to hurt anything.

Weird. I'm surprised that this is in the template. It's not important one way or the other.

...IntrinsicsInSystemPrivatecoreLibAnalyzer.Tests/IntrinsicsInSystemPrivateCoreLib.Tests.csproj

jkoritzinsky · 2023-05-15T17:46:03Z

src/libraries/System.Private.CoreLib/gen/IntrinsicsInSystemPrivateCoreLibAnalyzer.cs

+                return null;
+        }
+
+        private static INamedTypeSymbol[] GatherAndConditions(SemanticModel model, ExpressionSyntax expressionToDecompose)


This would be better done operating on IOperation instances than syntax, but that can be a later refactoring (as this does work).

src/libraries/System.Private.CoreLib/gen/IntrinsicsInSystemPrivateCoreLibAnalyzer.cs

…foImpl.ReadyToRun.cs Co-authored-by: Jeremy Koritzinsky <[email protected]>

…rivatecoreLibAnalyzer.Tests/IntrinsicsInSystemPrivateCoreLib.Tests.csproj Co-authored-by: Jeremy Koritzinsky <[email protected]>

…vateCoreLibAnalyzer.cs Co-authored-by: Jeremy Koritzinsky <[email protected]>

davidwrighton added 3 commits April 26, 2023 15:49

Remove BypassReadyToRunAttribute as it should no longer be needed for…

b246bd0

… intrinsics usages

Document new intrinsic handling behavior and enhance crossgen2 to res…

35105fb

…pect the new attribute

davidwrighton requested a review from MichalStrehovsky as a code owner April 27, 2023 19:11

ghost added the area-crossgen2-coreclr label Apr 27, 2023

ghost assigned davidwrighton Apr 27, 2023

davidwrighton mentioned this pull request Apr 27, 2023

Fix Avx2 helpers #85275

Merged

davidwrighton requested review from sbomer and tannergooding April 27, 2023 19:12

Fix build break

59a55e4

jkoritzinsky reviewed Apr 27, 2023

View reviewed changes

build-analysis bot mentioned this pull request Apr 28, 2023

Tracking issue for CI build timeouts #76454

Closed

sbomer reviewed Apr 28, 2023

View reviewed changes

Fix condition noted in code review

768d1a9

MihaZupan reviewed Apr 28, 2023

View reviewed changes

davidwrighton added 8 commits May 8, 2023 17:17

Update analyzer to detect more patterns, and attribute much of System…

5ac974c

….Private.CoreLib

Add pragmas and attributes to the packed span helpers

cdc8b3f

- These use a peculiar construct but its legal as its all containing in 1 method. If it got more complex than that, the analysis would be quite a bit harder

Rename attribute and fix docs

5573738

Merge branch 'main' of github.com:dotnet/runtime into IntrinsicsAnaly…

5134b86

…zerAndFixes

Add analyzer tests and fixup issues from merging with main branch

506f053

Disable tests on Mono due to Mono runtime bug

2e33ae4

Handle incorrect namespace for the CompExactlyDependsOnAttribute and …

8a69798

…don't pass a null InstructionSetSupport into the ShouldSkipCompilation logic, just feed it around everywhere.

davidwrighton requested review from MihaZupan, jkoritzinsky and sbomer May 12, 2023 03:02

sbomer approved these changes May 12, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/gen/IntrinsicsInSystemPrivateCoreLibAnalyzer.cs Outdated Show resolved Hide resolved

src/libraries/System.Private.CoreLib/gen/IntrinsicsInSystemPrivateCoreLibAnalyzer.cs Outdated Show resolved Hide resolved

davidwrighton added 2 commits May 15, 2023 09:12

Merge branch 'main' of github.com:dotnet/runtime into IntrinsicsAnaly…

919e0c3

…zerAndFixes

With the analyzer, negative checks are problematic, so re-order some …

4e413bd

…of them. Also we needed some new pragma warning disables to make the new logic work

Code review feedback

994ff3e

MihaZupan reviewed May 15, 2023

View reviewed changes

jkoritzinsky reviewed May 15, 2023

View reviewed changes

davidwrighton and others added 5 commits May 15, 2023 11:52

Update src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorIn…

8ed51a1

…foImpl.ReadyToRun.cs Co-authored-by: Jeremy Koritzinsky <[email protected]>

Update src/libraries/System.Private.CoreLib/tests/IntrinsicsInSystemP…

9442ebe

…rivatecoreLibAnalyzer.Tests/IntrinsicsInSystemPrivateCoreLib.Tests.csproj Co-authored-by: Jeremy Koritzinsky <[email protected]>

Update src/libraries/System.Private.CoreLib/gen/IntrinsicsInSystemPri…

fba92eb

…vateCoreLibAnalyzer.cs Co-authored-by: Jeremy Koritzinsky <[email protected]>

Update IntrinsicsInSystemPrivateCoreLibAnalyzer.cs

fcc38ca

Use auto-implemented get-only property

62c5e58

jkoritzinsky approved these changes May 15, 2023

View reviewed changes

davidwrighton merged commit 8a2aec1 into dotnet:main May 16, 2023

ghost locked as resolved and limited conversation to collaborators Jun 15, 2023

		// You can change these strings in the Resources.resx file. If you do not want your analyzer to be localize-able, you can use regular strings for Title and MessageFormat.
		// See https://github.com/dotnet/roslyn/blob/main/docs/analyzers/Localizing%20Analyzers.md for more on localization

	if (instructionSet.Architecture != architecture) continue;
	if (potentialVLinstructionSet.Architecture != architecture) continue;

Intrinsics analyzer and fixes #85481

Intrinsics analyzer and fixes #85481

Uh oh!

Conversation

davidwrighton commented Apr 27, 2023

Uh oh!

jkoritzinsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwrighton commented May 10, 2023

Uh oh!

sbomer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MihaZupan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone