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

JIT: Encode SIMD base type in VN for all HW intrinsics #105869

Merged
merged 5 commits into from
Aug 2, 2024

Conversation

jakobbotsch
Copy link
Member

This changes the VN funcs for all HW intrinsic functions to always have an extra parameter for the SIMD base type. Before this change it was based on the instruction list, however that is not always the right thing (e.g. #105721 is a bug because of that). We also kept it as part of the HW intrinsic table, but that requires manual maintenance and is easy to get wrong. Always encoding the type is much more simple and the diffs do not look too bad.

In .NET 10 we can decide if we want to opt some intrinsics into not being differentiated based on the SIMD base type. The easiest thing might be to always map those to have the same base SIMD type (e.g. CORINFO_TYPE_BYTE) so that we don't end up with differences in arities for some VN functions that are hard to reason about.

Fix #105721

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 2, 2024
@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @tannergooding

I went with your suggestion to just encode the SIMD base type for all HW intrinsics, and removed all the references to the old stuff. The diffs (from the run before I pushed the test) are miniscule so definitely seems like the right direction to go here.

@jakobbotsch jakobbotsch marked this pull request as ready for review August 2, 2024 15:34
@tannergooding
Copy link
Member

The diffs (from the run before I pushed the test) are miniscule so definitely seems like the right direction to go here.

👍, mostly looks to be around bitwise operations (and, or, xor, not, ternlog, etc). Users have the ability to manually workaround for these if its important in .NET 9 and mixing types to that degree is relatively rare, so I wouldn't expect most code to be pessimized by this. Should be easy to improve the select cases in .NET 10.

normalPair = vnStore->VNPairForFunc(tree->TypeGet(), func);
assert(vnStore->VNFuncArity(func) == 0);
}
// There are zero arg HWINTRINSICS operations that encode the result type, i.e. Vector128_AllBitSet
Copy link
Member

Choose a reason for hiding this comment

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

This comment is notably out of date, we produce GT_CNS_VEC for cases like AllBitsSet now. Off the top of my head, other zero arg intrinsics should also be getting converted to constant nodes and I'm not sure any cases actually exist that can hit this anymore.

Should be safe to leave in for .NET 9, but might be something we can cleanup early in .NET 10

@jakobbotsch jakobbotsch merged commit 014632b into dotnet:main Aug 2, 2024
117 of 123 checks passed
@jakobbotsch jakobbotsch deleted the fix-105721 branch August 2, 2024 20:16
@a74nh
Copy link
Contributor

a74nh commented Aug 5, 2024

We're getting failures on all the SVE tests. We suspect it's this PR, but just confirming...

  Starting test: /home/mikabl01/dotnet/runtime/artifacts/tests/coreclr/linux.arm64.Checked/Tests/Core_Root/corerun -p System.Reflection.Metadata.MetadataUpdater.IsSupported=false -p System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization=true ./artifacts/tests/coreclr/linux.arm64.Checked/JIT/HardwareInt  rinsics/HardwareIntrinsics_Arm_ro/HardwareIntrinsics_Arm_ro.dll GatherVectorFirstFaulting                                                                                                                                                                                                                                   
  ===================Running default===================                                                                                                                                                                                                                                                                       
  ------------------- {} -------------------                                                                                                                                                                                                                                                                                  
  Errors:                                                                                                                                                                                                                                                                                                                     
                                                                                                                                                                                                                                                                                                                              
  Assert failure(PID 3341648 [0x0032fd50], Thread: 3341648 [0x32fd50]): Assertion failed 'vnStore->VNFuncArity(func) == 1' in 'JIT.HardwareIntrinsics.Arm._Sve.SveGatherVectorVectorBasesTest__Sve_GatherVectorFirstFaulting_Bases_double_ulong:RunBasicScenario_UnsafeRead():this' during 'Do value numbering' (IL size 110;   hash 0x04f13b09; FullOpts)                                                                                                                                                                                                                                                                                                  
                                                                                                                                                                                                                                                                                                                              
      File: /home/mikabl01/dotnet/runtime/src/coreclr/jit/valuenum.cpp:12998                                                                                                                                                                                                                                                  
      Image: /home/mikabl01/dotnet/runtime/artifacts/tests/coreclr/linux.arm64.Checked/Tests/Core_Root/corerun   

@jakobbotsch
Copy link
Member Author

We're getting failures on all the SVE tests. We suspect it's this PR, but just confirming...

  Starting test: /home/mikabl01/dotnet/runtime/artifacts/tests/coreclr/linux.arm64.Checked/Tests/Core_Root/corerun -p System.Reflection.Metadata.MetadataUpdater.IsSupported=false -p System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization=true ./artifacts/tests/coreclr/linux.arm64.Checked/JIT/HardwareInt  rinsics/HardwareIntrinsics_Arm_ro/HardwareIntrinsics_Arm_ro.dll GatherVectorFirstFaulting                                                                                                                                                                                                                                   
  ===================Running default===================                                                                                                                                                                                                                                                                       
  ------------------- {} -------------------                                                                                                                                                                                                                                                                                  
  Errors:                                                                                                                                                                                                                                                                                                                     
                                                                                                                                                                                                                                                                                                                              
  Assert failure(PID 3341648 [0x0032fd50], Thread: 3341648 [0x32fd50]): Assertion failed 'vnStore->VNFuncArity(func) == 1' in 'JIT.HardwareIntrinsics.Arm._Sve.SveGatherVectorVectorBasesTest__Sve_GatherVectorFirstFaulting_Bases_double_ulong:RunBasicScenario_UnsafeRead():this' during 'Do value numbering' (IL size 110;   hash 0x04f13b09; FullOpts)                                                                                                                                                                                                                                                                                                  
                                                                                                                                                                                                                                                                                                                              
      File: /home/mikabl01/dotnet/runtime/src/coreclr/jit/valuenum.cpp:12998                                                                                                                                                                                                                                                  
      Image: /home/mikabl01/dotnet/runtime/artifacts/tests/coreclr/linux.arm64.Checked/Tests/Core_Root/corerun   

Seems likely, sorry about that. How do I run those tests?

@jakobbotsch
Copy link
Member Author

Although, the failing test was presumably added in #105595, so it might be an unlucky conflict with this PR (this PR went in shortly before that one).

@SwapnilGaikwad
Copy link
Contributor

Hi @jakobbotsch , the same issue is also observed for other intrinsics. I checked the Multiply, AddSaturate, SubtractSaturate.
You can run tests for the above intrinsics using the following steps.

  1. Build the tests - ./src/tests/build.sh checked -test:JIT/HardwareIntrinsics/HardwareIntrinsics_Arm_ro.csproj (Windows equivalent of the command)
  2. Run the tests -
DOTNET_TieredCompilation=0 $CORE_ROOT/corerun ./artifacts/tests/coreclr/linux.arm64.Checked/JIT/HardwareIntrinsics/HardwareIntrinsics_Arm_ro/HardwareIntrinsics_Arm_ro.dll Sve_Multiply

Here, CORE_ROOT=artifacts/tests/coreclr/linux.arm64.Checked/Tests/Core_Root

@github-actions github-actions bot locked and limited conversation to collaborators Sep 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JIT: VN does not encode base type for CreateScalar
4 participants