Skip to content
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.

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

[release/8.0-staging] Ensure we consistently broadcast the result of simd dot product #106031

Merged

Conversation

tannergooding
Copy link
Member

Backport of #105888 to release/8.0-staging

/cc @tannergooding

Customer Impact

Users with hardware that supports SSE3-SSSE3 (2004-2007) and using Vector2 can experience incorrect results in some cases as the result is not correctly replicated to all elements of the vector. This is not an issue with hardware that only supports SSE2 (our baseline ISA) or with hardware that supports SSE4.1+ (2007).

Regression

  • Yes
  • No

The bug was introduced in #81335 when an optimization was added to avoid redundant broadcast. It was missed that for SSE3-SSSE3 and for Vector2 in particular (but not for Vector3/4 or Vector64/128/256/512<T>), a slightly different code path was gone down under which the broadcast was not redundant.

Testing

Explicit tests covering the bug were added. Additional tests covering the repro for the other vector types were also added.

Risk

Low. This impacts a single type and only on relatively old hardware. Users can also see the failure on new hardware if they are doing testing and set the DOTNET_EnableSSE41=0 environment variable.

The effective fix here was to execute the same code path as was already being used by SSE2 hardware (the same baseline that NAOT, Crossgen, and ReadyToRun default to).

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 6, 2024
@tannergooding
Copy link
Member Author

/azp run runtime-coreclr jitstress-isas-x86

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

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

@tannergooding
Copy link
Member Author

@carlossanlop, looks like I can't run any outerloop CI legs due to a permissions issue. Is that expected?

This is impacting downlevel hardware, so running the stress job to explicitly validate those scenarios is important.

@carlossanlop
Copy link
Member

looks like I can't run any outerloop CI legs due to a permissions issue. Is that expected?

What should I be looking at?

@tannergooding
Copy link
Member Author

The runtime-coreclr jitstress-isas-x86 job.

All builds in it were cancelled with

##[error]Pipeline does not have permissions to use the referenced pool(s) NetCore-Svc-Public. For authorization details, refer to https://aka.ms/yamlauthz.

@carlossanlop
Copy link
Member

I shared this problem to FR. I tagged you.

@tannergooding
Copy link
Member Author

CC. @JulieLeeMSFT, @jeffschwMSFT for servicing approval

As per the template, this was a regression introduced in .NET 8 against a small subset of hardware that supports SSE3 but not SSE4.1

Copy link
Member

@JulieLeeMSFT JulieLeeMSFT left a comment

Choose a reason for hiding this comment

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

Approved. cc @jeffschwMSFT for servicing consider.

@JulieLeeMSFT JulieLeeMSFT changed the title Ensure we consistently broadcast the result of simd dot product [release/8.0-staging] Ensure we consistently broadcast the result of simd dot product Aug 12, 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.

lgtm. we will take for consideration in 8.0.x

@tannergooding tannergooding added the Servicing-consider Issue for next servicing release review label Aug 12, 2024
@JulieLeeMSFT JulieLeeMSFT added this to the 8.0.x milestone Aug 12, 2024
@carlossanlop
Copy link
Member

@tannergooding did this get approved by Tactics? I'm about to close the branch for the September Release. If this is not merged now, we will have to wait until October.

@tannergooding
Copy link
Member Author

Hasn't been taken to tactics yet, to my knowledge.

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Aug 13, 2024
@leecow leecow modified the milestones: 8.0.x, 8.0.9 Aug 13, 2024
@tannergooding tannergooding merged commit 42ababd into dotnet:release/8.0-staging Aug 14, 2024
136 of 140 checks passed
@tannergooding tannergooding deleted the backport-fix-99391 branch August 14, 2024 02:07
@carlossanlop carlossanlop modified the milestones: 8.0.9, 8.0.10 Aug 15, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 15, 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.

5 participants