Ensure the JIT view of InstructionSets is correct in the face of JitConfig switches #62420

tannergooding · 2021-12-05T18:29:18Z

This resolves #60035

Part of the problem in #60035 was that the VM wasn't tracking InstructionSet_Vector* and instead these were added by the JIT. The other problem was that EnableHWIntrinsic=0 wasn't correctly disabling the baseline ISA for the platform, which lead to an inconsistent world view on some APIs.

~~There are a few reasons why this isn't being done in the VM as suggested #60035 (comment).~~

Reading these flags in the VM is problematic for a few reasons namely that some of these flags control the "baseline" ISAs (X86Base, SSE, SSE2; ArmBase, AdvSimd) and only influence hardware intrinsics. To that end, the VM must report such ISAs as reported while the JIT still needs to support disabling them and having them correctly impact the actual codegen.

Likewise, we have cross-targeting AltJit where the VM won't be correct at all and same-target AltJit where you may want to view disassembly for ISAs your hardware doesn't support (which will become more prevalent as we try to support "latest" ISAs including AvxVnni which was added as "experimental" before hardware existed; or potential future support for SVE and AVX-512 where its unlikely devs may support it locally).

ghost · 2021-12-05T18:29:25Z

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

This resolves #60035

Part of the problem in #60035 was that the VM wasn't tracking InstructionSet_Vector* and instead these were added by the JIT. The other problem was that EnableHWIntrinsic=0 wasn't correctly disabling the baseline ISA for the platform, which lead to an inconsistent world view on some APIs.

There are a few reasons why this isn't being done in the VM as suggested #60035 (comment).

Reading these flags in the VM is problematic for a few reasons namely that some of these flags control the "baseline" ISAs (X86Base, SSE, SSE2; ArmBase, AdvSimd) and only influence hardware intrinsics. To that end, the VM must report such ISAs as reported while the JIT still needs to support disabling them and having them correctly impact the actual codegen.

Likewise, we have cross-targeting AltJit where the VM won't be correct at all and same-target AltJit where you may want to view disassembly for ISAs your hardware doesn't support (which will become more prevalent as we try to support "latest" ISAs including AvxVnni which was added as "experimental" before hardware existed; or potential future support for SVE and AVX-512 where its unlikely devs may support it locally).

Author:	tannergooding
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

tannergooding · 2021-12-05T18:34:21Z

CC. @echesakovMSFT, @jkotas

jkotas · 2021-12-05T18:47:35Z

Are the *.IsSupported properties guaranteed to return the same value for crossgened and JITed code after this change?

tannergooding · 2021-12-05T22:00:51Z

Are the *.IsSupported properties guaranteed to return the same value for crossgened and JITed code after this change?

Based on the failures, looks like no.

The VM (src/coreclr/vm) logic and JIT (src/coreclr/jit) logic are both now correct and once again match the behavior we had in .NET 5 and so compiling for the hardware the JIT is running on is correct.

I expect there is some issue with the crossgen logic here, likely the same that is causing: #61471. The crossgen VM needs to report what hardware it's targeting and likely also ensure that the relevant COMPlus_Enable* flags aren't present (or the JIT needs to know to ignore them for crossgen).

tannergooding · 2021-12-05T22:14:58Z

Notably, I'd also think that the JIT and Crossgen don't need to agree here.

Crossgen is supposed to mark what ISAs are used by a method. If a method uses an ISA that the hardware doesn't support, it has to rejit at which point whatever the JIT is targeting is fine.

If the hardware does support it, then it shouldn't matter what the JIT thinks because the code will still run fine. The one notable exception here may be AVX because that also controls the VEX encoding and by extension Vector256 support.

Therefore, if AVX is turned on and a method uses any SIMD instruction; then it will be emitted using the VEX encoding and it won't be usable on hardware that doesn't support AVX.

But aside from that, if the method is crossgen'd for SSE4.1 and the hardware supports SSE4.1, then even if the JIT says "no SSE4.1", the method will still execute fine.

jkotas · 2021-12-05T22:41:49Z

But aside from that, if the method is crossgen'd for SSE4.1 and the hardware supports SSE4.1, then even if the JIT says "no SSE4.1", the method will still execute fine.

This works fine for opportunistic ISA uses where the user code cannot observe the functional difference. It breaks down when the code can observe IsSupported property. For example, consider code like this and various interleavings of AOTed and JITed code:

void M1()
{
    if (Avx.IsSupported) M2();
}

void M2()
{
   if (!Avx.IsSupported) throw new InvalidOperationException();
}

We have seen patterns like this in real-world code.

tannergooding · 2021-12-05T22:50:45Z

How would you recommend handling the AltJit case?

That is, I'm running on x64 and want to get disassembly for Arm64? Should there be a new JIT/EE interface that asks for the ISA support for a given target platform or should the JIT assume "everything is supported" and replicate the JitConfig checks?

jkotas · 2021-12-05T23:09:33Z

I would either do nothing, or replicate the environment checks when the JIT runs as cross-compiling altjit.

I do not think it makes sense to introduce altjit specific methods on JIT/EE interface.

tannergooding · 2021-12-06T05:05:10Z

src/coreclr/inc/clrconfigvalues.h

-#define EXTERNAL_JitEnableAVX_Default 1
-#else // !(defined(TARGET_AMD64) || defined(TARGET_X86)
-#define EXTERNAL_JitEnableAVX_Default 0
-#endif // !(defined(TARGET_AMD64) || defined(TARGET_X86)
 RETAIL_CONFIG_DWORD_INFO(EXTERNAL_FeatureSIMD, W("FeatureSIMD"), EXTERNAL_FeatureSIMD_Default, "Enable SIMD intrinsics recognition in System.Numerics.dll and/or System.Numerics.Vectors.dll")


We need to do something about FeatureSIMD. Originally it was intended to be a workaround in case the System.Numerics.Vector* logic had issues (back before hardware intrinsics were even a concept).

However, given how critical SIMD is to the ABI and even basic operations in the JIT for x64 and Arm64; I think it might be better to actually remove the public facing flag here and leave it only a compile level switch for x86 Unix and Arm32 (the two targets where FEATURE_SIMD == 0 today).

If we don't remove it, then we need to also have the VM take it into account for certain ISAs (basically everything except X86Base, Lzcnt, and ArmBase; Bmi1 and Bmi2 are "scalar" but not included because they depend on EnableAvx=1 due to the VEX encoding).

-- If we are fine with removing flags, EXTERNAL_EnableSSE3_4 is also legacy and unnecessary as it has the same meaning as EnableSSE3

tannergooding · 2021-12-06T05:13:08Z

src/coreclr/jit/compiler.cpp

-#endif
-
-    instructionSetFlags = EnsureInstructionSetFlagsAreValid(instructionSetFlags);
-    opts.setSupportedISAs(instructionSetFlags);


Commenting here because its in compiler.cpp, but further down on https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/compiler.cpp#L5608-L5637, we have some logic for if (!info.compMatchedVM).

It'd be helpful to understand what this means for crossgen/r2r and if its better handled as simply if (info.altJit). In particular we want to support:

The ability to have "correct" instruction sets for the actual target when the VM doesn't match (i.e. cross-compiling for Arm64 on an x64 box; crossgen or otherwise)

To allow for an AltJit to generate disassembly for hardware you don't have (i.e. see and debug Arm.AdvSimd on x64; or even X86.Avx2 on an x64 box that only supports Avx)

tannergooding · 2022-01-04T15:38:59Z

@davidwrighton, @MichalStrehovsky there is something I'm not understanding about these flags for NativeAOT.

There are failures such as Vectors HW acceleration state unexpected - expected True, got False. Is there something additional expected for NativeAOT here?

tannergooding · 2022-01-04T16:40:44Z

Actually, I think I figured it out. I seem to have swapped the order of Set64BitInstructionSetVariants and EnsureValidInstructionSetSupport at some point and it was causing the flags to be incorrectly reported to the JIT.

tannergooding · 2022-01-04T16:55:28Z

After double checking the results reported by JIT\Regression\JitBlue\Runtime_34587, this is once again behaving as expected so hopefully CI passes now:

No Environment Variables

Supported x86 ISAs:
  AES:           True
  AVX:           True
  AVX2:          True
  BMI1:          True
  BMI2:          True
  FMA:           True
  LZCNT:         True
  PCLMULQDQ:     True
  POPCNT:        True
  SSE:           True
  SSE2:          True
  SSE3:          True
  SSE4.1:        True
  SSE4.2:        True
  SSSE3:         True
  X86Base:       True
Supported x64 ISAs:
  AES.X64:       True
  AVX.X64:       True
  AVX2.X64:      True
  BMI1.X64:      True
  BMI2.X64:      True
  FMA.X64:       True
  LZCNT.X64:     True
  PCLMULQDQ.X64: True
  POPCNT.X64:    True
  SSE.X64:       True
  SSE2.X64:      True
  SSE3.X64:      True
  SSE4.1.X64:    True
  SSE4.2.X64:    True
  SSSE3.X64:     True
  X86Base.X64:   True

COMPlus_EnableHWIntrinsic=0

Supported x86 ISAs:
  AES:           False
  AVX:           False
  AVX2:          False
  BMI1:          False
  BMI2:          False
  FMA:           False
  LZCNT:         False
  PCLMULQDQ:     False
  POPCNT:        False
  SSE:           False
  SSE2:          False
  SSE3:          False
  SSE4.1:        False
  SSE4.2:        False
  SSSE3:         False
  X86Base:       False
Supported x64 ISAs:
  AES.X64:       False
  AVX.X64:       False
  AVX2.X64:      False
  BMI1.X64:      False
  BMI2.X64:      False
  FMA.X64:       False
  LZCNT.X64:     False
  PCLMULQDQ.X64: False
  POPCNT.X64:    False
  SSE.X64:       False
  SSE2.X64:      False
  SSE3.X64:      False
  SSE4.1.X64:    False
  SSE4.2.X64:    False
  SSSE3.X64:     False
  X86Base.X64:   False

EnableSSE=0

Supported x86 ISAs:
  AES:           False
  AVX:           False
  AVX2:          False
  BMI1:          False
  BMI2:          False
  FMA:           False
  LZCNT:         True
  PCLMULQDQ:     False
  POPCNT:        False
  SSE:           False
  SSE2:          False
  SSE3:          False
  SSE4.1:        False
  SSE4.2:        False
  SSSE3:         False
  X86Base:       True
Supported x64 ISAs:
  AES.X64:       False
  AVX.X64:       False
  AVX2.X64:      False
  BMI1.X64:      False
  BMI2.X64:      False
  FMA.X64:       False
  LZCNT.X64:     True
  PCLMULQDQ.X64: False
  POPCNT.X64:    False
  SSE.X64:       False
  SSE2.X64:      False
  SSE3.X64:      False
  SSE4.1.X64:    False
  SSE4.2.X64:    False
  SSSE3.X64:     False
  X86Base.X64:   True

EnableSSE2=0

Supported x86 ISAs:
  AES:           False
  AVX:           False
  AVX2:          False
  BMI1:          False
  BMI2:          False
  FMA:           False
  LZCNT:         True
  PCLMULQDQ:     False
  POPCNT:        False
  SSE:           True
  SSE2:          False
  SSE3:          False
  SSE4.1:        False
  SSE4.2:        False
  SSSE3:         False
  X86Base:       True
Supported x64 ISAs:
  AES.X64:       False
  AVX.X64:       False
  AVX2.X64:      False
  BMI1.X64:      False
  BMI2.X64:      False
  FMA.X64:       False
  LZCNT.X64:     True
  PCLMULQDQ.X64: False
  POPCNT.X64:    False
  SSE.X64:       True
  SSE2.X64:      False
  SSE3.X64:      False
  SSE4.1.X64:    False
  SSE4.2.X64:    False
  SSSE3.X64:     False
  X86Base.X64:   True

etc, for the rest of the available EnableIsa=0 switches including for ARM64

tannergooding · 2022-01-04T19:31:30Z

Nope. @MichalStrehovsky, I'm definitely going to need help with NativeAOT here.

Even debugging in the flags look correct, but its still faulting for reasons I can't determine.

tannergooding · 2022-01-06T23:00:52Z

Ok, everything looks to be working as expected now including for CG2/ILC.

I've rebased onto main and updated JitBlue/Runtime_34587 to also display the results of IsHardwareAccelerated and validate they are correct in the right scenarios.

tannergooding · 2022-01-06T23:20:39Z

Thanks to both David and Michal for assisting here!

tannergooding · 2022-01-06T23:22:55Z

src/coreclr/tools/Common/JitInterface/ThunkGenerator/InstructionSetDesc.txt

+implication        ,X86   ,LZCNT     ,X86Base
 implication        ,X86   ,PCLMULQDQ ,SSE2
 implication        ,X86   ,POPCNT    ,SSE42
+implication        ,X86   ,Vector128 ,SSE
 implication        ,X86   ,Vector256 ,AVX
+implication        ,X86   ,AVXVNNI   ,AVX2


Couple new "implications" for x86. Nothing major, just ensuring AvxVnni and Lzcnt are correctly dependent on the ISAs they inherit from and ensure Vector128 is correctly marked as being dependent on Sse

tannergooding · 2022-01-06T23:23:15Z

src/coreclr/tools/Common/JitInterface/ThunkGenerator/InstructionSetDesc.txt

+implication        ,ARM64 ,Vector64  ,AdvSimd
+implication        ,ARM64 ,Vector128 ,AdvSimd


Same for ARM64. Just ensuring Vector64/128 are correctly marked as being dependent on AdvSimd.

tannergooding · 2022-01-06T23:27:05Z

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

+
+");
+
+            foreach (string architecture in _architectures)


It's probably easier to see these changes by looking at https://github.com/dotnet/runtime/pull/62420/files#diff-47b7c76a34a29a49750b1591fcf7ed6196296eb075ca71cc160b03ff5e717f49R21.

This basically just adds InstructionSet_X64, InstructionSet_X86, and InstructionSet_ARM64 enums that contain only the ISAs for that architecture. These aren't actually used by anything, they are just returned by three new properties on InstructionSetFlags to make it simpler to debug.

Without this, you'll end up seeing the ARM64_* enum values regardless of platform due to how the debugger displays enums with overlapping values. This was making it practically impossible to debug since you'd see ARM64_AdvSimd rather than X64_SSE for example.

tannergooding · 2022-01-06T23:33:51Z

CI never started, closing and reopening to try and force a refresh.

tannergooding · 2022-01-06T23:36:36Z

Ah, CI is broken due to #63480

…tructionSetSupport

… Avx2

…nfoInstructionSet

MichalStrehovsky

The crossgen2/nativeaot changes look good. Thank you for the debugging improvement!

tannergooding requested a review from MichalStrehovsky as a code owner December 5, 2021 18:29

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 5, 2021

jkotas requested a review from davidwrighton December 5, 2021 18:47

tannergooding force-pushed the fix-60035 branch 2 times, most recently from faddb15 to 1ea5ea2 Compare December 6, 2021 04:57

tannergooding commented Dec 6, 2021

View reviewed changes

runfoapp bot mentioned this pull request Dec 6, 2021

System.Runtime.Serialization.Xml.Tests failed #59926

Closed

tannergooding force-pushed the fix-60035 branch from 1ea5ea2 to ac8e18a Compare January 3, 2022 20:12

tannergooding force-pushed the fix-60035 branch from bd6611e to fddd424 Compare January 6, 2022 23:00

tannergooding commented Jan 6, 2022

View reviewed changes

tannergooding closed this Jan 6, 2022

tannergooding reopened this Jan 6, 2022

tannergooding added 9 commits January 6, 2022 20:03

Adding a regression test for dotnet#60035

283ce7c

Add the correct "ISA implications" for Vector64/128

d4a0438

Moving the COMPlus_Enable* HWIntrinsic ISA switches to the VM

cae62e8

Ensure Set64BitInstructionSetVariants is called before EnsureValidIns…

93d993a

…tructionSetSupport

Ensure opts.compSupportsISA isn't overwritten to 0

816cd1e

Ensure that Lzcnt is dependent on X86Base and AvxVnni is dependent on…

c36f9a3

… Avx2

Adding x64, x86, and Arm64 specific views to help with debugging CorI…

356d8bd

…nfoInstructionSet

Keep the JIT in charge of "artificial" ISAs

869f824

Update JitBlue/Runtime_34587 to also validate the xplat SIMD types

41bf5cd

tannergooding force-pushed the fix-60035 branch from fddd424 to 41bf5cd Compare January 7, 2022 04:03

MichalStrehovsky approved these changes Jan 7, 2022

View reviewed changes

tannergooding mentioned this pull request Jan 10, 2022

Make AvxVnni imply Avx2 #63566

Closed

davidwrighton approved these changes Jan 11, 2022

View reviewed changes

tannergooding merged commit 4f89b79 into dotnet:main Jan 12, 2022

ghost locked as resolved and limited conversation to collaborators Feb 11, 2022

tannergooding deleted the fix-60035 branch November 11, 2022 15:27

		implication ,ARM64 ,Vector64 ,AdvSimd
		implication ,ARM64 ,Vector128 ,AdvSimd

Ensure the JIT view of InstructionSets is correct in the face of JitConfig switches #62420

Ensure the JIT view of InstructionSets is correct in the face of JitConfig switches #62420

Uh oh!

Conversation

tannergooding commented Dec 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Dec 5, 2021

Uh oh!

tannergooding commented Dec 5, 2021

Uh oh!

jkotas commented Dec 5, 2021

Uh oh!

tannergooding commented Dec 5, 2021

Uh oh!

tannergooding commented Dec 5, 2021

Uh oh!

jkotas commented Dec 5, 2021

Uh oh!

tannergooding commented Dec 5, 2021

Uh oh!

jkotas commented Dec 5, 2021

Uh oh!

tannergooding Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

tannergooding Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

tannergooding commented Jan 4, 2022

Uh oh!

tannergooding commented Jan 4, 2022

Uh oh!

tannergooding commented Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No Environment Variables

COMPlus_EnableHWIntrinsic=0

EnableSSE=0

EnableSSE2=0

Uh oh!

tannergooding commented Jan 4, 2022

Uh oh!

tannergooding commented Jan 6, 2022

Uh oh!

tannergooding commented Jan 6, 2022

Uh oh!

tannergooding Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

tannergooding Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

tannergooding Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

tannergooding commented Jan 6, 2022

Uh oh!

tannergooding commented Jan 6, 2022

Uh oh!

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tannergooding commented Dec 5, 2021 •

edited

Loading

tannergooding Dec 6, 2021 •

edited

Loading

tannergooding commented Jan 4, 2022 •

edited

Loading