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

[ARM CPU] Add ACL executor for Transpose #17322

Merged
merged 32 commits into from
Jul 14, 2023
Merged

[ARM CPU] Add ACL executor for Transpose #17322

merged 32 commits into from
Jul 14, 2023

Conversation

allnes
Copy link
Contributor

@allnes allnes commented May 2, 2023 •
edited
Loading

  • Separate executors
  • Add ACL executor for transpose

@allnes allnes added category: CPU OpenVINO CPU plugin platform: arm OpenVINO on ARM / ARM64 labels May 2, 2023
@allnes allnes requested review from a team as code owners May 2, 2023 15:36
@allnes allnes added this to the 2023.1 milestone May 2, 2023
@akladiev
Copy link
Collaborator

This PR will be closed in a week because of 2 weeks of no activity.

@akladiev akladiev added the Stale label May 23, 2023
@allnes allnes removed the Stale label May 23, 2023
@alvoron alvoron requested a review from a team as a code owner June 20, 2023 16:29
@alvoron alvoron removed the request for review from a team June 20, 2023 16:29
EgorDuplensky
EgorDuplensky reviewed Jun 29, 2023
View reviewed changes
Copy link
Contributor

@EgorDuplensky EgorDuplensky left a comment

Choose a reason for hiding this comment

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

Could you please double check the possible regressions with dlb

EgorDuplensky
EgorDuplensky approved these changes Jul 5, 2023
View reviewed changes
src/plugins/intel_cpu/src/nodes/transpose.cpp Outdated
@@ -253,18 +273,25 @@ void Transpose::createPrimitive() {
} else if (getParentEdgeAt(INPUT_DATA_IDX)->getMemory().getDesc().hasLayoutType(LayoutType::ncsp) &&
std::find(optimizedOrders.begin(), optimizedOrders.end(), order) != optimizedOrders.end()) {
isOptimized = true;
execPtr = std::make_shared<TransposeRefExecutor>();
transposeParams.transposeExecution = TransposeParams::REF;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the factory is supposed to decide which executor to use / to force to use.
The idea is to avoid making such type of decisions in scope of Nodes

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed to fix in separate PR

@alvoron
Copy link
Contributor

alvoron commented Jul 13, 2023

Please do not merge the PR - I'd like to discuss the sequence of executor builders.

@maxnick
Copy link
Contributor

maxnick commented Jul 13, 2023

Please do not merge the PR - I'd like to discuss the sequence of executor builders.

It's better to use do_not_merge label then.

@alvoron
Copy link
Contributor

alvoron commented Jul 13, 2023

It's ready to merge as soon as CI is completed.

@maxnick
Copy link
Contributor

maxnick commented Jul 14, 2023

@alvoron the PR is ready to be merged, please make CI green.

@maxnick maxnick merged commit 238c7fa into openvinotoolkit:master Jul 14, 2023
alvoron added a commit to alvoron/openvino that referenced this pull request Jul 21, 2023
@allnes @alvoron
[CPU][ARM] Add ACL executor for Transpose (openvinotoolkit#17322)
ebb8093 
* separate executors + add acl executor fot transpose

* correct axisCast

* update transpose executors list

* update new changes

* enable tests

* fix fortting

* fixed test shapes and transpose generalization

* fixed different signedness error

* size_t usage in loop counters

* undo unwanted changes

* fixed comments

* added i8 and fp32 to blocked x86 tests

* fixed comments

* fixed comments

* extracted general reference executor from PermuteKernel

* fix mayiuse in JitTransposeExecutorBuilder::isSupported

* getDescWithType name refactoring

* refactoring

* removed 2nd executor creation in transpose node

* Moved RefOptimizedTranspose to the top

* fixed comments

---------

Co-authored-by: Aleksandr Voron <aleksandr.voron@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxnick maxnick maxnick approved these changes

@alvoron alvoron alvoron left review comments

@EgorDuplensky EgorDuplensky EgorDuplensky approved these changes

@dmitry-gorokhov dmitry-gorokhov Awaiting requested review from dmitry-gorokhov

Assignees

@maxnick maxnick

Labels
category: CPU OpenVINO CPU plugin platform: arm OpenVINO on ARM / ARM64
Projects
None yet
Milestone
2023.1
Development

Successfully merging this pull request may close these issues.
5 participants
@allnes @akladiev @alvoron @maxnick @EgorDuplensky