Skip to content

Conversation

@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Jan 17, 2024

Backport of #96951 to release/8.0-staging

/cc @tannergooding

Customer Impact

  • Customer reported
  • Found internally

This can result in incorrect computations if you are on a machine that pre-dates SSE4.1 support (a machine with a CPU from before ~Nov 2007)

Regression

  • Yes
  • No

On older hardware (pre-SSE4.1) use of the Create(Dot(...)) optimization would kick in for Vector2. However, since there are two elements, a multiply followed by a single pairwise addition is emitted and the operation isn't naturally broadcasting on this older hardware. This is in contrast to newer hardware where it can utilize the dot product instruction which can always broadcast the result.

Testing

A regression test covering the scenario was added.

Risk

Low. The issue is well understood and the hardware this impacts is increasingly small. For cloud providers, most default to AVX2 capabable hardware or newer (~Jun 2013). For consumer hardware, places such as the Steam Hardware Survey (see under Other Settings) report 99.57% of customers have SSE4.1 support.

This wasn't surfacing in scenarios such as crossgen/r2r because SSE4.1 support is opportunistically enabled by default.

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 17, 2024
@ghost
Copy link

ghost commented Jan 17, 2024

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

Issue Details

Backport of #96951 to release/8.0-staging

/cc @tannergooding

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@tannergooding tannergooding added the Servicing-consider Issue for next servicing release review label Jan 17, 2024
@tannergooding
Copy link
Member

CC. @EgorBo

@danmoseley
Copy link
Member

Does this affect Arm?

@tannergooding
Copy link
Member

Does this affect Arm?

No.

x86/x64 only supports V128 and therefore you get HorizontalAdd(vector, vector) producing the following:

[
    vector[0] + vector[1],
    vector[2] + vector[3],
    vector[0] + vector[1],
    vector[2] + vector[3]
]

This is problematic for Vector2 since it only uses elements 0 and 1 and so therefore the second element will be incorrect (typically 0).

However, Arm64 supports V64 and so you get AddPairwise(vector, vector) producing the following, so the result remains correct:

[
    vector[0] + vector[1],
    vector[0] + vector[1]
]

@jeffschwMSFT jeffschwMSFT removed the Servicing-consider Issue for next servicing release review label Jan 30, 2024
@tannergooding
Copy link
Member

@jeffschwMSFT, @JulieLeeMSFT was this not approved in servicing review or was there some other issue blocking it?

This was a customer reported codegen bug as indicated in the summary.

@jeffschwMSFT
Copy link
Member

This just needs a code review and then we can take it to tactics. I removed the servicing-consider as it was hanging out for multiple potential reviews.

@tannergooding
Copy link
Member

@EgorBo could you give the review here since you signed off on the original fix?

@tannergooding tannergooding added the Servicing-consider Issue for next servicing release review label Feb 1, 2024
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. we will take for consideration in 8.0.x

@rbhanda rbhanda added this to the 8.0.3 milestone Feb 8, 2024
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 8, 2024
@tannergooding
Copy link
Member

CC. @EgorBo, @dotnet/jit-contrib for merging. Monday February 12th is the Code Complete deadline for the March Release, so it needs to be merged by then (Julie asked that I let one of the JIT reviewers do the actual merge here).

@jeffschwMSFT jeffschwMSFT merged commit eded9a5 into release/8.0-staging Feb 9, 2024
@tannergooding tannergooding deleted the backport/pr-96951-to-release/8.0-staging branch February 9, 2024 20:30
@github-actions github-actions bot locked and limited conversation to collaborators Mar 12, 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 Servicing-approved Approved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants