Skip to content

[TOPI][Target] Add SVE specific convolution#14483

Merged
Mousius merged 1 commit intoapache:mainfrom
FranklandJack:conv2d_sve_sched
May 17, 2023
Merged

[TOPI][Target] Add SVE specific convolution#14483
Mousius merged 1 commit intoapache:mainfrom
FranklandJack:conv2d_sve_sched

Conversation

@FranklandJack
Copy link
Copy Markdown
Contributor

This commit will:

  • Expose SVE as a target feature for Arm(R) Cortex(R) A-Profile CPUs.
  • Update the compute definition of conv2d_spatial_pack_nhwc to defer to the LLVM backend for vectorization when compiling on an SVE enabled target for data tensors with unit width and height since this has been shown to be performant for wide vector architectures.

@tvm-bot
Copy link
Copy Markdown
Collaborator

tvm-bot commented Apr 4, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

  • No users to tag found in teams: topi, target See #10317 for details

Generated by tvm-bot

Comment thread src/target/parsers/aprofile.cc Outdated
Comment thread src/target/parsers/aprofile.cc
Comment thread python/tvm/topi/arm_cpu/conv2d_spatial_pack.py
Comment thread tests/cpp/target/parsers/aprofile_test.cc Outdated
This commit will:
* Expose SVE as a target feature for Arm(R) Cortex(R) A-Profile CPUs.
* Update the compute definition of `conv2d_spatial_pack_nhwc` to defer
  to the LLVM backend for vectorization when compiling on an SVE enabled
  target for data tensors with unit width and height since this has been
  shown to be performant for wide vector architectures.
* Add a target test to `cpptest` for Arm(R) Cortex(R) A-Profile CPUs
  which tests that the `has_sve` flag is set when the user explicitly
  passes the `"+sve"` attribute on target creation. Because SVE is
  optional on architecture versions 8.0 and later the test checks
  whether it is optionally set.
* Make some pre-existing read only target properties constant.

Change-Id: I7fef5c7e19da8aa140e61b28c26683c8467853fb
@neildhickey
Copy link
Copy Markdown
Contributor

LGTM!

Copy link
Copy Markdown
Contributor

@ekalda ekalda left a comment

Choose a reason for hiding this comment

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

Thanks @FranklandJack, LGTM!

@Mousius I believe your comments are addressed, do you want to take a look again?

Copy link
Copy Markdown
Member

@Mousius Mousius left a comment

Choose a reason for hiding this comment

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

@Mousius I believe your comments are addressed, do you want to take a look again?

Eek! Yep, looks good to me, apologies again for such latency - thanks for the ping @ekalda!

@Mousius Mousius merged commit 14be4bf into apache:main May 17, 2023
@Mousius
Copy link
Copy Markdown
Member

Mousius commented May 17, 2023

@FranklandJack this has now landed, thank you for this awesome patch and your patience with the reviews 😸

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.

5 participants