Ensure that the new Vector512 related ISAs are covered by ILLink Substitutions #83040

tannergooding · 2023-03-06T18:04:34Z

Now that the foundational PRs are in, they'll be several people iterating on adding Vector512 related intrinsics concurrently.

Having these classes defined up front helps reduce merge conflicts and other issues that would otherwise arise.

This makes progress towards #73604, #74813, and #76579

…titutions

dotnet-issue-labeler · 2023-03-06T18:04:40Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

ghost · 2023-03-06T18:04:53Z

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author:	tannergooding
Assignees:	-
Labels:	`linkable-framework`
Milestone:	-

tannergooding · 2023-03-06T18:09:20Z

Just looking at "current" System.Private.Corelib, I'm seeing that X86Base.IsSupported isn't being folded...

For example, BitOperations.Log2 is defined as:

// The 0->0 contract is fulfilled by setting the LSB to 1.
// Log(1) is 0, and setting the LSB for values > 1 does not change the log2 result.
value |= 1;

// value    lzcnt   actual  expected
// ..0001   31      31-31    0
// ..0010   30      31-30    1
// 0010..    2      31-2    29
// 0100..    1      31-1    30
// 1000..    0      31-0    31
if (Lzcnt.IsSupported)
{
    return 31 ^ (int)Lzcnt.LeadingZeroCount(value);
}

if (ArmBase.IsSupported)
{
    return 31 ^ ArmBase.LeadingZeroCount(value);
}

if (WasmBase.IsSupported)
{
    return 31 ^ WasmBase.LeadingZeroCount(value);
}

// BSR returns the log2 result directly. However BSR is slower than LZCNT
// on AMD processors, so we leave it as a fallback only.
if (X86Base.IsSupported)
{
    return (int)X86Base.BitScanReverse(value);
}

// Fallback contract is 0->0
return Log2SoftwareFallback(value);

This is being folded down to:

value |= 1U;
if (Lzcnt.IsSupported)
  return 31 ^ (int) Lzcnt.LeadingZeroCount(value);
if (true) { } // ArmBase.IsSupported
if (true) { } // WasmBase.IsSupported
return X86Base.IsSupported ? (int) X86Base.BitScanReverse(value) : BitOperations.Log2SoftwareFallback(value);

Note that X86Base.IsSupported is not itself a "constant true" even though its "always available". This is because we don't correspondingly have an ILLink.Substituations.X86.xml (which would cause X86Base, Sse, Sse2, and Vector128 to all fold to constant "true"), is that right?

tannergooding · 2023-03-06T18:11:07Z

Also noticed that HardwareIntrinsicsHelpers.Aot uses the same path for X86 and X64: https://github.com/dotnet/runtime/blob/main/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/HardwareIntrinsicHelpers.Aot.cs#L67-L71

However, XArchIntrinsicConstants.FromInstructionSet itself is only handling X64_* and not the X86_*. Should this be split into X86IntrinsicConstants and X64InstrinsicConstants?

CC. @MichalStrehovsky on this and the immediate above.

tannergooding · 2023-03-06T18:12:46Z

Also noting we have a src/mono/wasm/build/ILLink.Substitutions.WasmIntrinsics.xml and src/mono/wasm/build/ILLink.Substitutions.NoWasmIntrinsics.xml

They're in a separate location so they're easy to miss. It looks like they are being properly handled/processed however.

MichalStrehovsky · 2023-03-07T06:04:19Z

Note that X86Base.IsSupported is not itself a "constant true" even though its "always available". This is because we don't correspondingly have an ILLink.Substituations.X86.xml (which would cause X86Base, Sse, Sse2, and Vector128 to all fold to constant "true"), is that right?

I think so. Alternatively, we could have the IsSupported implemented as a constant true in the C# source code instead of the recursive call; that would work too. It's the reason why WasmBase gets stubbed out even though there's no substitution file for NoWasm (we have ILLink.Substitutions.NoArmIntrinsics.xml and ILLink.Substitutions.NoX86Intrinsics.xml, but no NoWasmIntrinsics.xml - I assume for historical reasons?).

However, XArchIntrinsicConstants.FromInstructionSet itself is only handling X64_* and not the X86_*. Should this be split into X86IntrinsicConstants and X64InstrinsicConstants?

IIRC these have the same underlying numerical constants so it doesn't matter. Either way X86 doesn't work with NativeAOT. We might as well delete the case statement.

tannergooding · 2023-03-07T17:14:43Z

I think so. Alternatively, we could have the IsSupported implemented as a constant true in the C# source code instead of the recursive call; that would work too. It's the reason why WasmBase gets stubbed out even though there's no substitution file for NoWasm (we have ILLink.Substitutions.NoArmIntrinsics.xml and ILLink.Substitutions.NoX86Intrinsics.xml, but no NoWasmIntrinsics.xml - I assume for historical reasons?).

@MichalStrehovsky Hmmm, we actually can't do this, unfortunately...

Basically while it is part of the baseline and "always supported", we also have a release mode config knob (DOTNET_EnableSSE2=0 for example) which allows users to disable the support.

Such knobs exist so that users can more easily test their various software fallback paths in addition to the intrinsic paths.

Replacing the method with returning => true (either in source or via IL substitutions) means that indirect calls to the IsSupported property (delegates, reflection, immediate window or other debug calls, etc) will report true when the actual code path may report false.

This is somewhat unfortunate as it means we can't actually trim as much as we'd like while still maintaining accuracy in the exposed APIs. Is there any other feature that might make it feasible to allow trimming here for crossgen2 (where such knobs don't exist) while still allowing indirect queries to work as expected?

tannergooding · 2023-03-07T22:05:30Z

CC. @dotnet/jit-contrib, @dotnet/avx512-contrib

Should be ready for review.

MichalStrehovsky · 2023-03-08T01:40:02Z

Is there any other feature that might make it feasible to allow trimming here for crossgen2 (where such knobs don't exist) while still allowing indirect queries to work as expected?

Crossgen2 should "trim" it based on my mental model. Crossgen2 currently pregenerates all public APIs in an assembly and privates are generated as-needed. So when crossgen2 compiles a method body, it will let RyuJIT do the substitution for IsSupported and that might eliminate the basic block plus native code for whatever private methods get called from that block. I don't see a way to trim beyond that if we want to keep the ability to turn off the baseline intrinsics.

tannergooding · 2023-03-08T02:48:24Z

Thanks! I think this should be generally ready for review then.

CC. @dotnet/jit-contrib, @dotnet/avx512-contrib

eerhardt · 2023-03-08T22:40:51Z

src/libraries/System.Runtime.Intrinsics/ref/System.Runtime.Intrinsics.cs

@@ -5210,7 +5274,7 @@ public new abstract partial class X64 : System.Runtime.Intrinsics.X86.Sse3.X64
            public static new bool IsSupported { get { throw null; } }
        }
    }
-    
+


(nit) was this generated with the tool? Some classes have a blank line between them, and some don't.

Yes, but also not entirely...

Several of the corelib facades have bits that weren't done by the tool (often attributes or simple ordering things) and which make generating this "properly" result in large diffs.

So, I typically cut out just the new bits from the generated file and paste them into the checked in copy to avoid that issue.

We should probably do the work to ensure they are in sync again, but it hasn't bubbled up in priority yet.

eerhardt · 2023-03-08T22:54:05Z

src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/X86/Avx512BW.cs

+        public static new bool IsSupported { get => IsSupported; }
+
+        [Intrinsic]
+        public new abstract class VL : Avx512F.VL


Do the public nested types need doc comments?

Yes, but it looks like "all" of them are currently missing for System.Runtime.Intrinsics.*.

I'll log an issue and gets this handled in a follow up PR later tonight.

eerhardt · 2023-03-08T22:57:28Z

src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.NoX86Intrinsics.xml

@@ -21,6 +24,42 @@
    <type fullname="System.Runtime.Intrinsics.X86.Avx2/X64">
      <method signature="System.Boolean get_IsSupported()" body="stub" value="false" />
    </type>
+    <type fullname="System.Runtime.Intrinsics.X86.Avx512BW">


Why do we need ILLink.Substitution entries for these when we also have the .PlatformNotSupported.cs versions that just return false? This xml file seems to be included in the same condition as when those .cs files are used.

Unsure actually. Maybe some legacy thing from back when they weren't guaranteed false?

It looks like I originally did it in ee12b54, but looking back I'm not sure it is needed. Since we are just following existing convention here, let's not block this PR on it.

eerhardt

The ILLink changes LGTM.

Ensure that the new Vector512 related ISAs are covered by ILLink Subs…

2cc8a13

…titutions

ghost added the linkable-framework Issues associated with delivering a linker friendly framework label Mar 6, 2023

ghost assigned tannergooding Mar 6, 2023

Define the Avx512F and derived classes

df34b30

eerhardt reviewed Mar 8, 2023

View reviewed changes

eerhardt approved these changes Mar 8, 2023

View reviewed changes

tannergooding mentioned this pull request Mar 8, 2023

Some hardware intrinsic ILLInk.Substitution entries may be unnecessary #83164

Closed

tannergooding merged commit 1b2eb12 into dotnet:main Mar 8, 2023

tannergooding deleted the issupported branch March 8, 2023 23:14

kotlarmilos mentioned this pull request Mar 17, 2023

[Perf] Linux/x64: 28 Regressions on 3/9/2023 2:48:42 AM dotnet/perf-autofiling-issues#13978

Closed

ghost locked as resolved and limited conversation to collaborators Apr 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that the new Vector512 related ISAs are covered by ILLink Substitutions #83040

Ensure that the new Vector512 related ISAs are covered by ILLink Substitutions #83040

tannergooding commented Mar 6, 2023 •

edited

Loading

dotnet-issue-labeler bot commented Mar 6, 2023

ghost commented Mar 6, 2023

tannergooding commented Mar 6, 2023

tannergooding commented Mar 6, 2023

tannergooding commented Mar 6, 2023

MichalStrehovsky commented Mar 7, 2023

tannergooding commented Mar 7, 2023

tannergooding commented Mar 7, 2023 •

edited

Loading

MichalStrehovsky commented Mar 8, 2023

tannergooding commented Mar 8, 2023

eerhardt Mar 8, 2023

tannergooding Mar 8, 2023

tannergooding Mar 8, 2023

eerhardt Mar 8, 2023

tannergooding Mar 8, 2023

tannergooding Mar 8, 2023

eerhardt Mar 8, 2023 •

edited

Loading

tannergooding Mar 8, 2023

eerhardt Mar 8, 2023

tannergooding Mar 8, 2023

eerhardt left a comment

Ensure that the new Vector512 related ISAs are covered by ILLink Substitutions #83040

Ensure that the new Vector512 related ISAs are covered by ILLink Substitutions #83040

Conversation

tannergooding commented Mar 6, 2023 • edited Loading

dotnet-issue-labeler bot commented Mar 6, 2023

ghost commented Mar 6, 2023

tannergooding commented Mar 6, 2023

tannergooding commented Mar 6, 2023

tannergooding commented Mar 6, 2023

MichalStrehovsky commented Mar 7, 2023

tannergooding commented Mar 7, 2023

tannergooding commented Mar 7, 2023 • edited Loading

MichalStrehovsky commented Mar 8, 2023

tannergooding commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

tannergooding commented Mar 6, 2023 •

edited

Loading

tannergooding commented Mar 7, 2023 •

edited

Loading

eerhardt Mar 8, 2023 •

edited

Loading