Skip to content

Conversation

@alvoron
Copy link
Contributor

@alvoron alvoron commented Jul 5, 2023

@github-actions github-actions bot added the category: CPU OpenVINO CPU plugin label Jul 5, 2023
continue;
}

#if defined(OV_CPU_WITH_ACL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add more details why it is not working specifically for convolution node?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmitry-gorokhov could you please propose the details I need to add here?

Choose a reason for hiding this comment

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

As I can wee previously where was no post-ops support for FP16 precision, but it was introduced in openvinotoolkit/oneDNN@f0229f0. So I would propose to remove FP16 fusion limitations added in this PR and see if it still allows to use ACL Conv and MatMul/FC impls.

@alvoron alvoron marked this pull request as ready for review July 11, 2023 16:49
@alvoron alvoron requested review from a team as code owners July 11, 2023 16:49
@dmitry-gorokhov dmitry-gorokhov added this to the 2023.0 milestone Jul 12, 2023
@dmitry-gorokhov dmitry-gorokhov modified the milestones: 2023.0, 2023.1 Jul 21, 2023
@dmitry-gorokhov dmitry-gorokhov self-assigned this Jul 21, 2023
@alvoron alvoron requested review from a team as code owners July 24, 2023 13:43
@github-actions github-actions bot added the category: build OpenVINO cmake script / infra label Jul 26, 2023
@dmitry-gorokhov
Copy link

Please also rebase PR on latest master and rebase merge conflicts

@github-actions github-actions bot added the category: IE Tests OpenVINO Test: plugins and common label Jul 27, 2023
@github-actions github-actions bot added category: Core OpenVINO Core (aka ngraph) category: CPP API OpenVINO CPP API bindings labels Jul 27, 2023
@github-actions github-actions bot removed category: Core OpenVINO Core (aka ngraph) category: CPP API OpenVINO CPP API bindings labels Jul 27, 2023
@github-actions github-actions bot removed the category: IE Tests OpenVINO Test: plugins and common label Jul 27, 2023
set(OV_CPU_WITH_ACL ON)
endif()

if (OV_CPU_WITH_ACL AND APPLE)

Choose a reason for hiding this comment

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

@EgorDuplensky @alvoron In fact condition which arch is used for ACL is determined here: https://github.com/openvinotoolkit/openvino/blob/master/src/plugins/intel_cpu/thirdparty/ACLConfig.cmake#L107. So once it will be changed this condition will become invalid.
Does it make sense to introduce intermidiate fp16 support variable inside ACLConfig which will contribute to OV_CPU_WITH_ACL_FP16 condition there?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we need to detect it directly in cpp code? E.g. when compiler enables FP16, it provides some macro like __ARM_FEATURE_FP16_VECTOR_ARITHMETIC

Copy link

@dmitry-gorokhov dmitry-gorokhov Jul 31, 2023

Choose a reason for hiding this comment

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

The intention is the following: the decision on FP16 support on product level (assuming CPU arch) should be made based on the plugin feedback. The plugin itself defines (based os some internal considirations) corresponding variable and bypass it all other parts of the product (including CPU third-parties).
We don't want to use HW specific flags, as long as HW level FP16 support is not equal to product level FP16 support. Such differentiation might be also usefull for HW-agnostic conditional compilation (e.g. to enable compilation w/o FP16 support).
Also CPU build w/o some backends (like ACL) might result in the fact we cannot support FP16 efficiently due to lack of optimized impls. For such cases FP16 support on CPU level should be disabled as well.

@dmitry-gorokhov dmitry-gorokhov merged commit e999198 into openvinotoolkit:master Jul 31, 2023
endif()

set(ARM_COMPUTE_TARGET_ARCH "${ARM_COMPUTE_TARGET_ARCH_DEFAULT}" CACHE STRING "Architecture for ARM ComputeLibrary")
set_property(CACHE ARM_COMPUTE_TARGET_ARCH PROPERTY STRINGS ${ARM_COMPUTE_TARGET_ARCHS})
Copy link
Contributor

Choose a reason for hiding this comment

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

why have you removed cache variables? In case use useful to select ARM arch via cmage-gui and see possible values for architectures

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: build OpenVINO cmake script / infra category: CPU OpenVINO CPU plugin platform: arm OpenVINO on ARM / ARM64

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants