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.

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

Implemented abs saturate, absolute compare intrinsics. #48361

Merged
merged 1 commit into from
Feb 18, 2021
Merged

Implemented abs saturate, absolute compare intrinsics. #48361

merged 1 commit into from
Feb 18, 2021

Conversation

naricc
Copy link
Member

@naricc naricc commented Feb 16, 2021 •
edited
Loading

Implement more ARM64 Intrinsic:

  • AbsSaturate
  • AbsScalar
  • AbsoluteCompareGreaterThan
  • AbsoluteCompareGreaterThanOrEqual
  • AbsoluteCompareLessThan
  • AbsoluteCompareLessThanOrEqual

Contributes to #42266

@naricc naricc force-pushed the naricc/more-simd branch 2 times, most recently from 32d0aa6 to 620aaef Compare February 16, 2021 21:13
@vargaz
Copy link
Contributor

vargaz commented Feb 16, 2021

Looks good otherwise.

CoffeeFlux
CoffeeFlux reviewed Feb 16, 2021
View reviewed changes
Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

Just nits, but the coding style throughout here is inconsistent and seems to have been done incorrectly in a few places and then copy-pasted. I fixed a few representative examples. There's also some problem with indentation that I'd like to see fixed.

src/mono/mono/mini/simd-intrinsics.c Outdated Show resolved Hide resolved
src/mono/mono/mini/simd-intrinsics.c Outdated Show resolved Hide resolved
src/mono/mono/mini/simd-intrinsics.c Outdated Show resolved Hide resolved
src/mono/mono/mini/simd-intrinsics.c Show resolved Hide resolved
src/mono/mono/mini/simd-intrinsics.c Outdated Show resolved Hide resolved
src/mono/mono/mini/simd-intrinsics.c Outdated Show resolved Hide resolved
src/mono/mono/mini/simd-intrinsics.c Outdated Show resolved Hide resolved
CoffeeFlux
CoffeeFlux reviewed Feb 16, 2021
View reviewed changes
Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

Few more you missed.

src/mono/mono/mini/simd-intrinsics.c Outdated Show resolved Hide resolved
src/mono/mono/mini/simd-intrinsics.c Outdated Show resolved Hide resolved
src/mono/mono/mini/simd-intrinsics.c Outdated Show resolved Hide resolved
src/mono/mono/mini/simd-intrinsics.c Outdated Show resolved Hide resolved
src/mono/mono/mini/simd-intrinsics.c Outdated Show resolved Hide resolved
src/mono/mono/mini/simd-intrinsics.c Outdated Show resolved Hide resolved
src/mono/mono/mini/simd-intrinsics.c Outdated Show resolved Hide resolved
src/mono/mono/mini/simd-intrinsics.c Outdated Show resolved Hide resolved
fanyang-mono
fanyang-mono approved these changes Feb 17, 2021
View reviewed changes
Copy link
Member

@fanyang-mono fanyang-mono 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 approved these changes Feb 17, 2021
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 - with couple of minor suggestions. I am also wondering how are these APIs tested? Does it use the tests in https://github.com/dotnet/runtime/tree/1d9e50cb4735df46d3de0cee5791e97295eaf588/src/tests/JIT/HardwareIntrinsics?

src/mono/mono/mini/mini.h Outdated Show resolved Hide resolved
src/mono/mono/mini/simd-intrinsics.c Show resolved Hide resolved
@imhameed
Copy link
Contributor

imhameed commented Feb 17, 2021 •
edited
Loading

LGTM - with couple of minor suggestions. I am also wondering how are these APIs tested? Does it use the tests in https://github.com/dotnet/runtime/tree/1d9e50cb4735df46d3de0cee5791e97295eaf588/src/tests/JIT/HardwareIntrinsics?

They should. That work is being done in #48397.

We've been manually testing with small test programs on arm64 hardware in the interim.

fanyang-mono
fanyang-mono reviewed Feb 17, 2021
View reviewed changes
src/mono/mono/mini/mini.h Outdated
Comment on lines 2889 to 2900
SIMD_OP_LLVM_I8ABS_SATURATE,
SIMD_OP_LLVM_I16ABS_SATURATE,
SIMD_OP_LLVM_I32ABS_SATURATE,
SIMD_OP_LLVM_I64ABS_SATURATE,
SIMD_OP_LLVM_I4FABSOLUTE_COMPARE_GREATER_THAN,
SIMD_OP_LLVM_DABSOLUTE_COMPARE_GREATER_THAN,
SIMD_OP_LLVM_FABSOLUTE_COMPARE_GREATER_THAN_OR_EQUAL,
SIMD_OP_LLVM_DABSOLUTE_COMPARE_GREATER_THAN_OR_EQUAL,
SIMD_OP_LLVM_FABSOLUTE_COMPARE_LESS_THAN,
SIMD_OP_LLVM_DABSOLUTE_COMPARE_LESS_THAN,
SIMD_OP_LLVM_FABSOLUTE_COMPARE_LESS_THAN_OR_EQUAL,
SIMD_OP_LLVM_DABSOLUTE_COMPARE_LESS_THAN_OR_EQUAL,
Copy link
Member

Choose a reason for hiding this comment

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

I had been using SIMD_OP_ARM64_. You are using SIMD_OP_LLVM_. We should talk about the naming convention and agree on a set of rules.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I think calling them SIMD_OP_ARM64 makes more sense since this particular file is not llvm only (although they don't actually get intensified). I think I will fix that in a follow up PR since other ones need to be renamed also.

Copy link
Member

Choose a reason for hiding this comment

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

Sure. No problem.

@naricc naricc merged commit 2b6046c into dotnet:master Feb 18, 2021
@naricc naricc deleted the naricc/more-simd branch February 18, 2021 16:34
@ghost ghost locked as resolved and limited conversation to collaborators Mar 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@CoffeeFlux CoffeeFlux CoffeeFlux left review comments

@imhameed imhameed imhameed left review comments

@fanyang-mono fanyang-mono fanyang-mono approved these changes

@vargaz vargaz vargaz approved these changes

@kunalspathak kunalspathak kunalspathak approved these changes

@SamMonoRT SamMonoRT SamMonoRT approved these changes

@EgorBo EgorBo Awaiting requested review from EgorBo

@lambdageek lambdageek Awaiting requested review from lambdageek

Assignees
No one assigned
Labels
area-Codegen-LLVM-mono
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@naricc @vargaz @imhameed @CoffeeFlux @kunalspathak @SamMonoRT @fanyang-mono