Skip to content

Vector128.WithElement codegen regression in .NET 9.0 #115348

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
May 8, 2025
Merged

Vector128.WithElement codegen regression in .NET 9.0 #115348

merged 10 commits into from
May 8, 2025
+74 −86

Conversation

kendall1997
Copy link
Contributor

Description

Fixes #108197.

Removes the handling of NI_Vector*_WithElement as a user call and mirrors the behavior of GetElement to emit the spill, set the value and then reload the vector.

@Copilot Copilot AI review requested due to automatic review settings May 6, 2025 20:51
@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 6, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 6, 2025
Copilot
Copilot AI reviewed May 6, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

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 fixes the handling of NI_Vector*_WithElement intrinsics so that user calls are removed and the behavior is aligned with GetElement (spill, set value, then reload).

  • Removed the NI_Vector*_WithElement user call handling in Rationalizer.
  • Updated lowering, codegen, and tree construction functions to correctly manage non-constant index cases by using spill temporary variables and performing range checks.

Reviewed Changes

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

Show a summary per file
File Description
src/coreclr/jit/rationalize.cpp Removed the special handling for NI_Vector*_WithElement to mirror GetElement behavior.
src/coreclr/jit/lowerxarch.cpp Added special handling for non-constant op2 by obtaining a SIMD temp var and deferring containment.
src/coreclr/jit/hwintrinsicxarch.cpp Removed the early return branch for non-constant index handling in favor of later processing.
src/coreclr/jit/hwintrinsiccodegenxarch.cpp Added a branch for non-constant op2 that stores the vector, updates the element, and reloads it.
src/coreclr/jit/gentree.cpp Updated node creation to perform a range check for non-constant op2 values before constructing the intrinsic node.

src/coreclr/jit/lowerxarch.cpp
@@ -5653,7 +5653,14 @@ GenTree* Lowering::LowerHWIntrinsicWithElement(GenTreeHWIntrinsic* node)
GenTree* op2 = node->Op(2);
GenTree* op3 = node->Op(3);

assert(op2->OperIsConst());
if (!op2->OperIsConst())
Copy link
Preview

Copilot AI May 6, 2025

Choose a reason for hiding this comment

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

Consider adding an inline comment explaining why a SIMD initialization temp variable is obtained when op2 is not a constant for improved clarity.

Copilot uses AI. Check for mistakes.

src/coreclr/jit/hwintrinsiccodegenxarch.cpp Outdated
unsigned offs = compiler->lvaFrameAddress(simdInitTempVarNum, &isEBPbased);

#if !FEATURE_FIXED_OUT_ARGS
if (!isEBPbased)
Copy link
Preview

Copilot AI May 6, 2025

Choose a reason for hiding this comment

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

Consider clarifying inline how the offset 'offs' is adjusted for non-EBP-based stacks to ensure the correct offset calculation across platforms.

Copilot uses AI. Check for mistakes.

src/coreclr/jit/gentree.cpp
@@ -27959,6 +27953,20 @@ GenTree* Compiler::gtNewSimdWithElementNode(
#error Unsupported platform
#endif // !TARGET_XARCH && !TARGET_ARM64

int immUpperBound = getSIMDVectorLength(simdSize, simdBaseType) - 1;
Copy link
Preview

Copilot AI May 6, 2025

Choose a reason for hiding this comment

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

Add an explanatory comment regarding the rationale for using 'getSIMDVectorLength(simdSize, simdBaseType) - 1' as the upper bound to aid future maintainability.

Copilot uses AI. Check for mistakes.

@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

@kendall1997
Copy link
Contributor Author

@dotnet-policy-service agree company="Intel"

@kendall1997 kendall1997 changed the title Kendall/vector128.with element Vector128.WithElement codegen regression in .NET 9.0 May 6, 2025
@build-analysis build-analysis bot mentioned this pull request May 7, 2025
Open
3 tasks
@kendall1997
Copy link
Contributor Author

@dotnet/intel

tannergooding
tannergooding approved these changes May 7, 2025
View reviewed changes
Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

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

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

@build-analysis build-analysis bot mentioned this pull request May 8, 2025
Open
kunalspathak
kunalspathak approved these changes May 8, 2025
View reviewed changes
Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM

@kunalspathak kunalspathak merged commit e42c072 into dotnet:main May 8, 2025
104 of 106 checks passed
@AndyAyersMS
Copy link
Member

Looks like perhaps jit-format failed to run? I am getting formatting errors in this file in my PR

clang-format: there are formatting errors in D:\a\runtime\runtime\runtime\src\coreclr\jit\hwintrinsiccodegenxarch.cpp
At Line 2004 Before:
            GetEmitter()->emitIns_S_R(ins_Store(simdType, compiler->isSIMDTypeLocalAligned(simdInitTempVarNum)),
                                        emitTypeSize(simdType), op1Reg, simdInitTempVarNum, 0);
After:
            GetEmitter()->emitIns_S_R(ins_Store(simdType, compiler->isSIMDTypeLocalAligned(simdInitTempVarNum)),
                                      emitTypeSize(simdType), op1Reg, simdInitTempVarNum, 0);
At Line 2008 Before:
            GetEmitter()->emitIns_ARX_R(ins_Move_Extend(op3->TypeGet(), false),   // Store
After:
            GetEmitter()->emitIns_ARX_R(ins_Move_Extend(op3->TypeGet(), false), // Store
At Line 2009 Before:

@AndyAyersMS AndyAyersMS mentioned this pull request May 9, 2025
Draft
@kendall1997 kendall1997 mentioned this pull request May 9, 2025
Open
@kendall1997
Copy link
Contributor Author

Created #115416 to try fix the issue.

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

Copilot code review Copilot Copilot left review comments

@tannergooding tannergooding tannergooding approved these changes

@kunalspathak kunalspathak kunalspathak approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Vector128.WithElement codegen regression in .NET 9.0
4 participants
@kendall1997 @AndyAyersMS @tannergooding @kunalspathak