JIT: Allow baseline intrinsics in JIT code unconditionally by saucecontrol · Pull Request #116196 · dotnet/runtime

saucecontrol · 2025-06-01T18:43:05Z

Under the current JIT rules, the config knobs meant for testing HWIntrinsics APIs and fallback code also impact codegen for ordinary casts. Some casts are implemented directly in codegen, where we freely emit SSE/SSE2 code with no ISA checks, while the casts implemented in lowering create HWIntrinsic nodes that end up hitting ISA validation checks in codegen and must, therefore, be gated by compOpportunisticallyDependsOn.

This PR adds the baseline ISAs to the supported set unconditionally so that we can freely use the HWIntrinsics in any JIT phase. It continues to honor config knobs (specifically DOTNET_EnableHWIntrinsic) when resolving managed HWIntrinsic methods so that user code is unaffected by the change.

In addition to the fgMorphExpandCast simplification in this PR, this change will allow

removal of the double->int/uint cast helpers
moving ulong->floating fallback cast implementation from codegen to lowering, which will open up additional optimization.

dotnet-policy-service · 2025-06-01T18:43:54Z

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

saucecontrol · 2025-06-01T21:30:05Z

cc @tannergooding

Diffs are a slight improvement over the other approach.

I tried adding the baseline ISAs unconditionally VM-side, but that breaks R2R under DOTNET_EnableHWIntrinsic=0 since the VM doesn't know the baseline ISAs should be off and thus disagrees with JIT. This approach splits the difference.

tannergooding · 2025-06-04T17:40:06Z

+    instructionSetFlags.AddInstructionSet(InstructionSet_Vector128);
+    instructionSetFlags.AddInstructionSet(InstructionSet_X86Base);
+#ifdef TARGET_AMD64
+    instructionSetFlags.AddInstructionSet(InstructionSet_X86Base_X64);
+#endif // TARGET_AMD64


Why here and not before the preferredVectorBitWidth checks?

No particular reason other than leaving the preferredVectorBitWidth comments close to the others since they're kind of related. I can move it if you prefer.

It would make a difference on if preferredVectorBitWidth is 0 rather than 128, right?

Do we meaningfully need to support that given the changes?

Ah yeah, none of Vector### can be added until we've checked the VM ISA set to resolve preferredVectorBitWidth. We can move up the X86Base adds if that reads better to you.

I think it's fine, just wanted to confirm it's intentional.

A comment explaining we're adding it and why its after would be goodness though, as future readers might forget the context.

Added a couple more comments.

tannergooding · 2025-06-04T17:41:28Z

    bool IsBaselineSimdIsaSupported()
    {
 #ifdef FEATURE_SIMD
-#if defined(TARGET_XARCH)
-        CORINFO_InstructionSet minimumIsa = InstructionSet_X86Base;
-#elif defined(TARGET_ARM64)
-        CORINFO_InstructionSet minimumIsa = InstructionSet_AdvSimd;
+#if defined(TARGET_XARCH) || defined(TARGET_ARM64)
+        return true;
 #elif defined(TARGET_LOONGARCH64)
        // TODO: supporting SIMD feature for LoongArch64.
        assert(!"unimplemented yet on LA");
-        CORINFO_InstructionSet minimumIsa = 0;
+        return false;
 #else
 #error Unsupported platform
 #endif // !TARGET_XARCH && !TARGET_ARM64 && !TARGET_LOONGARCH64
-
-        return compOpportunisticallyDependsOn(minimumIsa);
 #else
        return false;
 #endif
    }


Is this needed? Can we just remove it or otherwise change the name to something more meaningful?

We could get rid of this entirely if we got rid of FEATURE_SIMD, or rather made the assumption TARGET_XARCH and TARGET_ARM64 always imply it. I removed all calls to it except those that aren't guarded by FEATURE_SIMD, so it's at least const true or false based on that define.

Should we rename it to something like IfFeatureSimd() or similar instead or just change those paths to be properly guarded by #ifdef FEATURE_SIMD?

Either of those is fine with me. I was kind of thinking a separate effort to clean up FEATURE_SIMD and FEATURE_HW_INTRINSICS defines would be in order, particularly since Arm64 is inconsistent in whether those are checked at all. I don't even know if you can build without them currently.

I don't even know if you can build without them currently.

We really don't want to build without them for Arm64 or x64 at this point.

Realistically it's for Unix x86 (which I think someone is working on fixing) and for community platforms that are in their initial onboarding stages.

I'm definitely in favor of a cleanup/simplification around the ifdefs.

They're on for x86 Unix as well since #111861, and AFAIK, it's all working.

I forgot this is called from a few places that are cross-architecture, to make unrolling decisions and such. I'm not sure if there's a name that makes more sense for all cases, but I can at least remove the calls from the platform-specific files for xarch and Arm64.

Turns out I was able to get rid of this cleanly. As long as we don't bring back any non-SIMD builds for xarch or Arm64, it's not needed.

tannergooding · 2025-06-06T19:50:05Z

CC. @dotnet/jit-contrib, @EgorBo for secondary review

saucecontrol · 2025-06-06T23:31:00Z

Latest diff run shows a small TP improvement on Arm64, and the same asm diffs as before (mostly inlining of cast logic, and SIMD stack zeroing).

This change will only have diffs with DOTNET_EnableHWIntrinsic=0, so presumably these are test projects that explicitly set it.

EgorBo · 2025-06-06T23:41:44Z

LGTM but let's spin a few outerloops JIC

Does it mean EnableSSE/EnableSSE2 are no-op knobs now?

EgorBo · 2025-06-06T23:41:53Z

/azp list

EgorBo · 2025-06-06T23:43:55Z

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress-isas-x86, runtime-coreclr jitstress-isas-arm, Fuzzlyn

azure-pipelines · 2025-06-06T23:44:20Z

Azure Pipelines successfully started running 4 pipeline(s).

saucecontrol · 2025-06-06T23:54:28Z

Does it mean EnableSSE/EnableSSE2 are no-op knobs now?

Those were removed in #115983. It was always the case that the only way to disable X86Base and ArmBase was with EnableHWIntrinsic -- just now it's the full baseline group.

allow any baseline intrinsics in JIT

d79f664

github-actions Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 1, 2025

dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Jun 1, 2025

formatting

7fe0322

saucecontrol marked this pull request as ready for review June 1, 2025 21:30

tannergooding reviewed Jun 4, 2025

View reviewed changes

Comment thread src/coreclr/jit/hwintrinsic.cpp

tannergooding reviewed Jun 4, 2025

View reviewed changes

Comment thread src/coreclr/jit/lowerarmarch.cpp Outdated

tannergooding approved these changes Jun 6, 2025

View reviewed changes

tannergooding requested a review from EgorBo June 6, 2025 19:50

saucecontrol added 3 commits June 6, 2025 13:55

remove IsBaselineSimdIsaSupported

26dce53

update comments

4de2edd

Merge remote-tracking branch 'upstream/main' into baseline-isa

69e3187

This comment was marked as resolved.

Sign in to view

This was referenced Jun 7, 2025

Test failure: baseservices/exceptions/stackoverflow/stackoverflowtester/stackoverflowtester.cmd #110173

Open

Test failure: JIT/Regression/JitBlue/DevDiv_255294/DevDiv_255294/DevDiv_255294.cmd #106675

Closed

tannergooding merged commit 39ecf52 into dotnet:main Jun 7, 2025
184 of 192 checks passed

saucecontrol deleted the baseline-isa branch June 7, 2025 14:45

saucecontrol mentioned this pull request Jun 7, 2025

Remove double -> int/uint cast helpers #116401

Closed

github-actions Bot locked and limited conversation to collaborators Jul 8, 2025

Conversation

saucecontrol commented Jun 1, 2025

Uh oh!

dotnet-policy-service Bot commented Jun 1, 2025

Uh oh!

saucecontrol commented Jun 1, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saucecontrol Jun 4, 2025 • 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!

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!

tannergooding commented Jun 6, 2025

Uh oh!

saucecontrol commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EgorBo commented Jun 6, 2025

Uh oh!

EgorBo commented Jun 6, 2025

Uh oh!

This comment was marked as resolved.

EgorBo commented Jun 6, 2025

Uh oh!

azure-pipelines Bot commented Jun 6, 2025

Uh oh!

saucecontrol commented Jun 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

saucecontrol Jun 4, 2025 •

edited

Loading

saucecontrol commented Jun 6, 2025 •

edited

Loading