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

cpu: aarch64: Expand ARM SVE support in jit_uni_softmax #1786

Merged

Conversation

deepeshfujitsu
Copy link
Contributor

@deepeshfujitsu deepeshfujitsu commented Jan 18, 2024

Description

Enhancement: Expand ARM SVE support in jit_uni_softmax

This commit enhances the existing ARM SVE support in jit_uni_softmax to include additional vector length compatibility. The changes made are for implementation of different ARM SVE vector length.

Major Code changes:
• Introduce a new struct to accommodate the extended ARM SVE vector length, similar to the sve_512 struct.
• Modify the get_horizontal_op function accordingly for enhanced vector length.

Checklist

General

  • [✓] Do all unit and benchdnn tests (make test and make test_benchdnn_*) pass locally for each commit? Yes
    Test output is same with and without this commit.
  1. make test output:

    99% tests passed, 2 tests failed out of 160
    
     Total Test time (real) =  50.68 sec
     
     The following tests FAILED:
             144 - test_graph_unit_dnnl_conv_usm_cpu (Failed)
             149 - test_graph_unit_dnnl_large_partition_usm_cpu (Failed)
     Errors while running CTest
     Output from these tests are in: /home/deepesh/pull_request/oneDNN/build/Testing/Temporary/LastTest.log
     Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.
     make: *** [Makefile:71: test] Error 8
    
  2. make test_benchdnn_* output:

     make: *** No rule to make target 'test_benchdnn_*'.  Stop.
    
  3. make test_benchdnn_softmax_ci_cpu/fast output:

     tests:6144 passed:1384 skipped:4736 mistrusted:24 unimplemented:0 invalid_arguments:0 failed:0 listed:0
     total: 13.70s; fill: 1.39s (10%); compute_ref: 0.47s (3%); compare: 0.87s (6%);
    
  • [✓] Have you formatted the code using clang-format? Yes

cc: @kawakami-k , @davsva01

@@ -665,6 +665,283 @@ struct jit_softmax_t<sve_512> : public jit_softmax_base_t<sve_512> {
}
}; // namespace aarch64

template <>
struct jit_softmax_t<sve_256> : public jit_softmax_base_t<sve_256> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for repeating the code for SVE256 - which is the same as SVE512 from Lines 389? Can there be just one jit_softmax_t and then there be entries for SVE512, 256 etc? (See #1445)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cfRod, there is no specific reason for repeating the code for SVE256; it was done for faster development.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok fair enough

Choose a reason for hiding this comment

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

Thanks @cfRod

@densamoilov
Copy link
Contributor

@cfRod, do you have any comments from your side?

Copy link
Contributor

@cfRod cfRod left a comment

Choose a reason for hiding this comment

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

Suggested for a refactor but happy to approve either way.

@abhijain1204fujitsu
Copy link

@densamoilov , Hello the changes are approved from @cfRod
Can you please confirm if this PR can be progressed for merger.

@dzarukin dzarukin merged commit da52cd6 into uxlfoundation:main Feb 6, 2024
@dzarukin
Copy link
Contributor

dzarukin commented Feb 6, 2024

Thank you for the contribution!

@abhijain1204fujitsu
Copy link

Thank you @densamoilov @dzarukin for your quick response and merging the PR

@vpirogov vpirogov added this to the v3.5 milestone Feb 13, 2024
@vpirogov vpirogov added the platform:cpu-aarch64 Codeowner: @oneapi-src/onednn-cpu-aarch64 label May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform:cpu-aarch64 Codeowner: @oneapi-src/onednn-cpu-aarch64
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants