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

NEON : Complex operations from Armv8.3-a #1077

Merged
merged 29 commits into from Oct 17, 2023

Conversation

wewe5215
Copy link
Contributor

@wewe5215 wewe5215 commented Oct 16, 2023

This pull request includes initial implementations and corresponding test cases listed below

  • cadd_rot
  • cmla_lane
  • cmla_rot_lane

Sorry for the typo in commit fa9a14d.
It is [Neon] Add vcmla_rot270_lane_f{16/32} and vcmla_rot270_laneq_f{16/32} and vcmlaq_rot270_lane_f{16/32} and vcmlaq_rot270_laneq_f{16/32}

@mr-c
Copy link
Collaborator

mr-c commented Oct 16, 2023

About the binary operations and 16-bit floating points: they might work when SIMDE_FLOAT16_API is SIMDE_FLOAT16_API_FLOAT16 or SIMDE_FLOAT16_API_FP16;

@wewe5215
Copy link
Contributor Author

wewe5215 commented Oct 16, 2023

@mr-c

Thanks for your help. I have fixed it and pushed the code again!

simde/arm/neon/cmla_lane.h Outdated Show resolved Hide resolved
Comment on lines 74 to 75
result = simde_float16x4_from_private(r_);
return result;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Combine these lines

Comment on lines 78 to 67
#if defined(SIMDE_ARM_NEON_A32V8_ENABLE_NATIVE_ALIASES)
#undef vcmla_lane_f16
#define vcmla_lane_f16(r, a, b, lane) simde_vcmla_lane_f16(r, a, b, lane)
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#if defined(SIMDE_ARM_NEON_A32V8_ENABLE_NATIVE_ALIASES)
#undef vcmla_lane_f16
#define vcmla_lane_f16(r, a, b, lane) simde_vcmla_lane_f16(r, a, b, lane)
#endif
#if defined(SIMDE_ARM_NEON_A32V8_ENABLE_NATIVE_ALIASES)
#undef vcmla_lane_f16
#define vcmla_lane_f16(r, a, b, lane) simde_vcmla_lane_f16(r, a, b, lane)
#endif

Comment on lines 52 to 53
simde_float32x4_private r_ =
simde_float32x4_to_private(simde_vcvt_f32_f16(r)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This style is more readable

Suggested change
simde_float32x4_private r_ =
simde_float32x4_to_private(simde_vcvt_f32_f16(r)),
simde_float32x4_private r_ = simde_float32x4_to_private(
simde_vcvt_f32_f16(r)),

Comment on lines 47 to 48


Copy link
Collaborator

Choose a reason for hiding this comment

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

In general, there are several instances of too many blank lines

Suggested change

Comment on lines 990 to 993

// simde_float32x4_t r = simde_vcmlaq_rot180_laneq_f32(r_, a, b,
// test_vec[i].lane); simde_test_arm_neon_write_f32x4(2, r,
// SIMDE_TEST_VEC_POS_LAST);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please tidy up your test code as well; thank you!

Suggested change
// simde_float32x4_t r = simde_vcmlaq_rot180_laneq_f32(r_, a, b,
// test_vec[i].lane); simde_test_arm_neon_write_f32x4(2, r,
// SIMDE_TEST_VEC_POS_LAST);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello! I've cleaned up the code and reformatted it using Clang-Format with the LLVM style. Apologies for the coding style and redundant comments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the clean up. SIMDe does not (yet) have an official clang-format style. I see that the LLVM style still uses ColumnLimit: 80, which I find to be too narrow.

I will add that guidance to https://github.com/simd-everywhere/simde/wiki/Coding-Style for future contributors

Copy link
Collaborator

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

It looks like all the compiler issues are fixed (I think the rpm-build:fedora-rawhide-i386 failure is not your fault). Please fix the formatting errors and merge/re-base on the latest commits in https://github.com/simd-everywhere/simde/tree/master ; I'll then merge this PR! Thank you @wewe5215 !

@wewe5215
Copy link
Contributor Author

@mr-c Hello! I have reformatted the code and rebased it on the latest commit. I really appreciate your patience!

@mr-c mr-c enabled auto-merge (squash) October 17, 2023 11:34
@mr-c mr-c merged commit d08d67c into simd-everywhere:master Oct 17, 2023
80 of 81 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants