Skip to content

Conversation

@MichalPetryka
Copy link
Contributor

Fixes #89338

@ghost ghost added 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 labels Jul 22, 2023
@ghost
Copy link

ghost commented Jul 22, 2023

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

Issue Details

Fixes #89338

Author: MichalPetryka
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@danmoseley
Copy link
Member

Do we need a test?

@BruceForstall
Copy link
Contributor

@tannergooding @dotnet/avx512-contrib PTAL

@BruceForstall BruceForstall added the avx512 Related to the AVX-512 architecture label Jul 24, 2023
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.

Change is correct, but adding tests would still be desirable.

Looks like the operator shift tests are missing entirely, which would explain why this wasn't caught earlier.

You'd need to add the relevant entries to https://github.com/dotnet/runtime/blob/main/src/tests/Common/GenerateHWIntrinsicTests/GenerateHWIntrinsicTests_General.cs and likely clone the existing BinOp template to have a BinOpImm variant (much as exists for the method cases)

@tannergooding
Copy link
Member

@MichalPetryka, will you be able to add the necessary tests for this or do you need someone to pick it up?

@MichalPetryka
Copy link
Contributor Author

@MichalPetryka, will you be able to add the necessary tests for this or do you need someone to pick it up?

I've tried looking into creating a new template for the tests but I had a bit of trouble understanding what's the proper way to change that, I'd like somebody else to take care of this.

@tannergooding
Copy link
Member

CC. @fanyang-mono, seems there's a bug in the Mono shift logic where it isn't masking the shiftAmount.

It needs to be amount & ((sizeof(T) * 8) - 1):

  • byte/sbyte: x & 7
  • short/ushort: x & 15
  • int/uint/float: x & 31
  • long/ulong/double: x & 63

@tannergooding
Copy link
Member

Logged #89868 for the other bug that's being worked around.

@fanyang-mono
Copy link
Member

Please disable the failing tests on Build osx-x64 Release AllSubsets_Mono_Interpreter_RuntimeTests monointerpreter and open an issue for it and assign it to me. I will look into it later.

@JulieLeeMSFT
Copy link
Member

It is just a few days away from RC1 snap. What is the status of this PR? @MichalPetryka, @tannergooding.

@JulieLeeMSFT JulieLeeMSFT added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 10, 2023
@tannergooding
Copy link
Member

@JulieLeeMSFT, should be ready for merge now that tests were added and the Arm64 issue was resolved in #90051

It needs sign-off from @dotnet/jit-contrib

@tannergooding tannergooding merged commit 6112028 into dotnet:main Aug 11, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 10, 2023
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 avx512 Related to the AVX-512 architecture community-contribution Indicates that the PR has been added by a community member needs-author-action An issue or pull request that requires more info or actions from the author.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Invalid codegen for Vector512 arithmetic right shift operator (>>)

7 participants