Skip to content

APX - Improve NDD on latest Intel Hardware - #132727

Draft
kendall1997 wants to merge 9 commits into
dotnet:mainfrom
kendall1997:Kendall/ndd-without-memory-operand-public
Draft

kendall1997 wants to merge 9 commits into
dotnet:mainfrom
kendall1997:Kendall/ndd-without-memory-operand-public

Conversation

@kendall1997

Copy link
Copy Markdown
Contributor

This pull request disables memory forms of NDD instructions for optimal performance on Intel CPUs with APX support.

Instruction selection and code generation improvements:

  • src/coreclr/jit/codegenxarch.cpp: Updated the eligibility check for NDD instructions in genCodeForBinary to exclude cases where the second operand is sourced from memory, ensuring NDD is used only for register and immediate sources.
  • src/coreclr/jit/emitxarch.cpp: Modified emitIns_BASE_R_R_RM to disable NDD instructions when the RM source operand is in memory, defaulting to a mov+op sequence instead.

Copilot AI lite review requested due to automatic review settings August 24, 2026 23:36
@kendall1997
kendall1997 marked this pull request as draft August 24, 2026 23:36
@github-actions github-actions Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 24, 2026
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Aug 24, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Copilot AI 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.

Pull request overview

This PR adjusts x64 JIT instruction selection/emission to avoid generating the APX NDD (EVEX.ND) memory-source form, forcing a mov + binary-op sequence when the second source operand would come from memory.

Changes:

  • Tightened NDD eligibility in CodeGen::genCodeForBinary to exclude cases where op2 is sourced from memory.
  • Updated emitter::emitIns_BASE_R_R_RM to disable NDD when the RM operand is sourced from memory, falling back to mov+op.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/coreclr/jit/codegenxarch.cpp Prevents NDD selection when operand 2 is a memory source in genCodeForBinary.
src/coreclr/jit/emitxarch.cpp Prevents NDD move-elision + NDD emission when the RM source operand is memory in emitIns_BASE_R_R_RM.
Suppressed comments (1)

src/coreclr/jit/codegenxarch.cpp:1070

  • The new comment is Intel-specific ("latest Intel processors") but the condition only checks DoJitUseApxNDD (JitConfig.EnableApxNDD + encodability) and operand placement, not CPU vendor. This can be misleading documentation if APX/NDD ever becomes relevant beyond Intel.

Consider rewording the comment to describe the policy (disable memory-source form) without hard-coding a vendor claim unless the code actually checks for it.

        // On the latest Intel processors with APX, the memory form of NDD instructions has to be turned off by
        // default for optimal hardware performance, so keep NDD only for register and immediate sources.
        eligibleForNDD = emit->DoJitUseApxNDD(ins) && !op2->isUsedFromMemory();

Comment thread src/coreclr/jit/codegenxarch.cpp Outdated
Comment on lines +1068 to +1070
// On the latest Intel processors with APX, the memory form of NDD instructions has to be turned off by
// default for optimal hardware performance, so keep NDD only for register and immediate sources.
eligibleForNDD = emit->DoJitUseApxNDD(ins) && !op2->isUsedFromMemory();
Comment thread src/coreclr/jit/emitxarch.cpp Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@kendall1997
kendall1997 marked this pull request as ready for review August 25, 2026 19:22
Copilot AI review requested due to automatic review settings August 25, 2026 19:22
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI 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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread src/coreclr/jit/codegenxarch.cpp Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 25, 2026 19:32

Copilot AI 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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread src/coreclr/jit/emitxarch.cpp Outdated
Comment on lines 10471 to 10475
// Disable the memory-source form of NDD (EVEX.ND) instructions for performance reasons; fall back to the
// mov+op sequence when the RM source is in memory.
bool useApxNdd = DoJitUseApxNDD(ins) && !rmOp->isUsedFromMemory();

if (emitIns_Mov(INS_mov, attr, targetReg, regOp->GetRegNum(), true, useApxNdd) && useApxNdd)
@kendall1997

Copy link
Copy Markdown
Contributor Author

Hello @dotnet/intel, this PR is ready for your review! Thank you.

Comment thread src/coreclr/jit/codegenxarch.cpp Outdated
Comment on lines +1068 to +1069
// For performance, avoid the memory-source form of EVEX.ND; keep NDD only for register and immediate sources.
eligibleForNDD = emit->DoJitUseApxNDD(ins) && !op2->isUsedFromMemory();

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.

Can this be elaborated on a bit?

Is this saying it's avoiding ins dst, op1, [op2]? Can you elaborate more on why this is necessary and why it is or isn't a problem for ins dst, [op2] or for SIMD instructions?

This is going to bloat codegen and rather seems like it should be impacting lowering/lsra to ensure preferencing is correct, as otherwise we shouldn't have marked the operand as contained at all.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @tannergooding, the tuning is based on performance results we observed on NVL. Similar tuning changes have also been upstreamed in both GCC and LLVM:

Thanks for your comment, I'll update the PR soon with the latest version of the change for your review!

The memory-source form of APX NDD (`ins dst, src1, [mem]`) is not
profitable, so it is never selected; the legacy `mov dst, src1` +
`ins dst, [mem]` pair is used instead. Register and immediate NDD
forms are unchanged.

Move the decision out of emitIns_BASE_R_R_RM into its two callers,
genCodeForBinary and genCodeForMul, through a new
DoJitUseApxNDD(ins, rmOp) overload, and pass it in explicitly. The
emitter asserts the caller's choice, and that the fallback `mov`
cannot overwrite an address register of the memory operand. LSRA
guarantees the latter by modelling these nodes as read-modify-write.

No codegen change when APX NDD is disabled (the default).
For read-modify-write nodes, BuildRMWUses marks op2 delay-free because
the legacy sequence writes the destination before reading op2. The APX
NDD form reads both sources first, so the constraint is not needed when
codegen emits NDD.

For an integer sub with a register op2, skip the delay-free when NDD is
enabled for sub and op1 is a register-candidate local that stays live
after the node. The destination can't take op1's register there, so the
JIT already emitted the three-register NDD form; now the destination may
take op2's register. Codegen emits NDD `sub dst, op1, op2` with
dst == op2, and the emitter allows it. The op1 preference is unchanged,
and a contained op2 keeps its delay-free.

No codegen change when APX NDD is disabled (the default).
Move the LSRA check that drops the op2 delay-free for APX NDD sub out
of the else-if chain into a separate check under TARGET_AMD64, keyed
on delayUseOperand. The condition is unchanged.

In genCodeForBinary, let a non-commutative op whose destination got
op2's register fall through to the three-register path, which already
emits NDD. On the non-NDD path, a noway_assert keeps release builds
from emitting a mov that would overwrite op2 if LSRA and codegen ever
disagree.

No codegen change.

This branch has not been deployed

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

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants