Skip to content

Set minimum ISAs when compiling native code#130556

Open
MichalPetryka wants to merge 4 commits into
dotnet:mainfrom
MichalPetryka:patch-67
Open

Set minimum ISAs when compiling native code#130556
MichalPetryka wants to merge 4 commits into
dotnet:mainfrom
MichalPetryka:patch-67

Conversation

@MichalPetryka

Copy link
Copy Markdown
Contributor

Match native compiler configuration with .NET 11.

For MSVC see x64 and ARM64, for Unixes see GCC.

@am11 Is this the right place for this? I found no usages of /arch or -march anywhere so I just put this here.
cc @jkotas @tannergooding @EgorBo any reason why we wouldn't do this?

@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Jul 11, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib
See info in area-owners.md if you want to be subscribed.

@am11

am11 commented Jul 11, 2026

Copy link
Copy Markdown
Member

I think it'd be better to place them near:

add_compile_options(-march=armv7-a)
if(ARM_SOFTFP)
add_definitions(-DARM_SOFTFP)
add_compile_options(-mfloat-abi=softfp)
endif(ARM_SOFTFP)
endif(CLR_CMAKE_HOST_UNIX_ARM)
if(CLR_CMAKE_HOST_UNIX_ARMV6)
add_compile_options(-mfpu=vfp)
add_definitions(-DCLR_ARM_FPU_CAPABILITY=0x0)
add_compile_options(-march=armv6zk)
add_compile_options(-mcpu=arm1176jzf-s)
add_compile_options(-mfloat-abi=hard)
endif(CLR_CMAKE_HOST_UNIX_ARMV6)
if(CLR_CMAKE_HOST_UNIX_RISCV64)
add_compile_options(-march=rv64gc)
add_compile_options(-mabi=lp64d)
endif(CLR_CMAKE_HOST_UNIX_RISCV64)

@tannergooding

Copy link
Copy Markdown
Member

I believe we explicitly wanted to wait at least 1 release so that any users got a clear error message and we didn't see big perf wins for the JIT or VM, with the GC already opportunistically using things like LSE

But, I'll defer to Jan

@jkotas

jkotas commented Jul 12, 2026

Copy link
Copy Markdown
Member

Related to #118101

@jkotas

jkotas commented Jul 12, 2026

Copy link
Copy Markdown
Member

I think it'd be better to place them near:

Yes, it makes sense to have all baselines together.

endif()
elseif (CLR_CMAKE_TARGET_ARCH_ARM64)
if(CLR_CMAKE_HOST_WIN32)
add_compile_options($<$<COMPILE_LANGUAGE:C,CXX>:/arch:armv8.0+lse>)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should set LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT define as well

@jkotas

jkotas commented Jul 12, 2026

Copy link
Copy Markdown
Member

I believe we explicitly wanted to wait at least 1 release so that any users got a clear error message

If the unmanaged runtime is compiled with the higher baseline,we will likely crash with invalid instruction before we get a chance to print the good error message.

I think this is fine change for .NET 12. I would prefer the good error message in .NET 11 so that it is easier to diagnose issues from the higher baseline if there are any.

@MichalPetryka

Copy link
Copy Markdown
Contributor Author

Do we want to keep the PR open and wait with it for 12 or close it for now?

@jkotas

jkotas commented Jul 13, 2026

Copy link
Copy Markdown
Member

It is fine to keep it open.

@tannergooding tannergooding added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 13, 2026
@tannergooding tannergooding added this to the 12.0.0 milestone Jul 13, 2026
@tannergooding tannergooding self-assigned this Jul 13, 2026
@tannergooding

Copy link
Copy Markdown
Member

Assigning myself to track getting it merged once we open main for .NET 12 changes.

Comment thread eng/native/configureoptimization.cmake
Comment thread eng/native/configureoptimization.cmake Outdated
@EgorBo

EgorBo commented Jul 13, 2026

Copy link
Copy Markdown
Member

BTW, this also would need to check if we still manually outline LSE on win-arm64 or it constant folds ISA checks we do by hand

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
@jkotas

jkotas commented Jul 13, 2026

Copy link
Copy Markdown
Member

this also would need to check if we still manually outline LSE on win-arm64 or it constant folds ISA checks we do by hand

Defining LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT should take care of it. I agree we should check that it works as expected. We can also delete Windows-specific code for !LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT

@github-actions

github-actions Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "115e220012753eae9a5d64de02fb9b9e46552872",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "a7ed27ba3f7a0a32634be47079ac659909797806",
  "last_reviewed_commit": "115e220012753eae9a5d64de02fb9b9e46552872",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "a7ed27ba3f7a0a32634be47079ac659909797806",
  "last_recorded_worker_run_id": "29682979489",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "115e220012753eae9a5d64de02fb9b9e46552872",
      "review_id": 4730587542
    }
  ]
}

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Holistic Review

Motivation: The PR raises the minimum ISA baseline used when compiling the native runtime to match the documented .NET 11 minimum hardware requirements: x86-64-v2 / SSE4.2 on x64 and ARMv8.0 + LSE on ARM64. This lets the C/C++ compiler emit newer instructions (e.g., LSE atomics, SSE4.2) directly instead of conservatively targeting the old baseline.

Approach: A new block is appended to eng/native/configureoptimization.cmake that adds -march=x86-64-v2 (Unix) / /arch:SSE4.2 (MSVC) for AMD64/I386, and /arch:armv8.0+lse for Windows ARM64. The change is small and localized. Note the PR is intentionally marked NO-MERGE — per the discussion it is deferred to .NET 12 (main opening), and reviewers agreed .NET 11 should first ship the clear "unsupported hardware" error message before the native runtime itself is compiled with a higher baseline (otherwise an unsupported CPU crashes with an illegal-instruction fault before the diagnostic can print). The placement was also questioned; @jkotas/@am11 suggested consolidating all baselines near the existing arch blocks in configurecompiler.cmake.

Summary: The direction is agreed-upon, but the current patch has two correctness gaps worth resolving before merge: (1) -march=x86-64-v2 is applied to the 32-bit CLR_CMAKE_TARGET_ARCH_I386 target where it is not a valid flag, and the Windows side has no corresponding x86 handling; and (2) the Windows ARM64 armv8.0+lse branch does not define LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT, which the runtime's hand-written write-barrier code uses to select LSE vs. legacy paths — a point already flagged by @EgorBo and @jkotas in the discussion. Both are detailed inline. Verdict: needs changes before merge; otherwise reasonable and on an agreed path for .NET 12.

Detailed Findings

Both actionable findings are provided as inline comments:

  • 32-bit x86 (I386) gets an invalid -march=x86-64-v2 flag and has no MSVC x86 equivalent — see inline comment on the -march=x86-64-v2 line.
  • Windows ARM64 /arch:armv8.0+lse does not define LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT, risking inconsistent assumptions with the manual LSE write-barrier paths — see inline comment on the armv8.0+lse line.

Non-blocking observations (not inline, per discussion):

  • Consider consolidating these baselines with the existing architecture flag blocks in eng/native/configurecompiler.cmake (as suggested by @am11/@jkotas) rather than in configureoptimization.cmake, which otherwise only handles -O//O optimization levels.
  • src/native/libs/CMakeLists.txt carries its own duplicated optimization settings (with a TODO to unify); the new ISA baseline is not applied there, so libraries built through that path won't pick up the higher baseline. Worth confirming that's intended.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 83.4 AIC · ⌖ 11.1 AIC · ⊞ 10K

if(CLR_CMAKE_HOST_WIN32)
add_compile_options($<$<COMPILE_LANGUAGE:C,CXX>:/arch:SSE4.2>)
else()
add_compile_options(-march=x86-64-v2)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

-march=x86-64-v2 is being applied to CLR_CMAKE_TARGET_ARCH_I386 (32-bit x86), but x86-64-v2 is a 64-bit micro-architecture level. GCC/Clang reject it for a 32-bit target (-m32), so this branch will fail to compile the x86 build. If the intent is to also raise the 32-bit x86 baseline, use an explicit flag such as -msse4.2 (and matching /arch:SSE2-level handling on Windows, since /arch:SSE4.2 isn't a valid MSVC x86 switch either). Alternatively, drop CLR_CMAKE_TARGET_ARCH_I386 from this condition if 32-bit x86 is intentionally left at its current baseline.

endif()
elseif (CLR_CMAKE_TARGET_ARCH_ARM64)
if(CLR_CMAKE_HOST_WIN32)
add_compile_options($<$<COMPILE_LANGUAGE:C,CXX>:/arch:armv8.0+lse>)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As raised by @EgorBo/@jkotas in the PR discussion: enabling armv8.0+lse by default on Windows ARM64 changes the effective ISA baseline for the runtime, but this branch does not also define LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT. That macro currently gates the hand-written LSE-vs-legacy write barrier paths (src/coreclr/runtime/arm64/WriteBarriers.asm/.S) and is only set for OSX-Arm64 today (eng/native/configurecompiler.cmake). Without defining it here, the compiler may emit LSE atomics while the manual runtime paths still assume they might be unavailable, leaving inconsistent assumptions. Define LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT alongside /arch:armv8.0+lse so the manual ISA-check code folds consistently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants