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

Update of --config=mkl_aarch64 with Compute Library support #47775

Merged
merged 10 commits into from Mar 25, 2021

Conversation

alenik01
Copy link
Contributor

@alenik01 alenik01 commented Mar 13, 2021

This PR introduces the support of Compute Library for the Arm Architecture (ACL) as part of Bazel build for mkl_aarch64 build config. The code was developed together with @joeramsay
The PR is related to

This PR introduces the support of Compute Library for the Arm
Architecture (ACL) as part of Bazel build.

Signed-off-by: Aleksandr Nikolaev <aleksandr.nikolaev@arm.com>
@alenik01 alenik01 requested a review from penpornk as a code owner March 13, 2021 00:37
@google-ml-butler google-ml-butler bot added the size:L CL Change Size: Large label Mar 13, 2021
@google-cla google-cla bot added the cla: yes label Mar 13, 2021
@alenik01
Copy link
Contributor Author

@agramesh1 @gzmkl please have a look.

@gbaned gbaned self-assigned this Mar 14, 2021
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Mar 14, 2021
@penpornk penpornk added the comp:mkl MKL related issues label Mar 15, 2021
@gbaned
Copy link
Contributor

gbaned commented Mar 19, 2021

@alenik01 Can you please resolve conflicts? Thanks!

@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label Mar 19, 2021
@google-cla
Copy link

google-cla bot commented Mar 19, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Mar 19, 2021
@alenik01
Copy link
Contributor Author

alenik01 commented Mar 19, 2021

@gbaned, GitHub GUI for the conflict resolution fails to pass google-cla bot for some reason. Would force push be the best way to proceed here?

@penpornk
Copy link
Member

@alenik01 Please don't force push.

@google-cla I have verified that the commits are from the same user (@alenik01). Manually setting CLA to yes.

@penpornk penpornk added cla: yes and removed cla: no labels Mar 19, 2021
Copy link
Contributor

@agramesh1 agramesh1 left a comment

Choose a reason for hiding this comment

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

Thanks @alenik01 for the PR.
Also some of the files have missing newlines, can you fix those.

tensorflow/core/kernels/mkl/mkl_conv_ops.cc Show resolved Hide resolved
tensorflow/tensorflow.bzl Show resolved Hide resolved
third_party/mkl/build_defs.bzl Outdated Show resolved Hide resolved
PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Mar 19, 2021
@tensorflowbutler tensorflowbutler removed the stat:awaiting response Status - Awaiting response from author label Mar 21, 2021
@gbaned
Copy link
Contributor

gbaned commented Mar 21, 2021

@alenik01 Can you please check @agramesh1's comments and keep us posted ? Thanks!

@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label Mar 21, 2021
Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! I have minor comments.

tensorflow/workspace2.bzl Outdated Show resolved Hide resolved
tensorflow/workspace2.bzl Show resolved Hide resolved
deps = ["arm_compute_core"],

visibility = ["//visibility:public"],
)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please add a new line at the end of file. Same for other files.

Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

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

Since this PR doesn't seem to need any more significant changes, I'm going to approve this now and make the remaining minor fixes myself to save time. Thank you again for the PR!

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Mar 23, 2021
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 23, 2021
@alenik01
Copy link
Contributor Author

@penpornk, thanks. My main concern is how this PR stacks with recently merged PR #47745. If the functionality will be broken, then what way would be the best to proceed, open another PR for the fix?

@alenik01
Copy link
Contributor Author

@penpornk, arm_any leads to the following conflict during the build:

ERROR: Analysis of target '//tensorflow/tools/pip_package:build_pip_package' failed; build aborted: /tensorflow/tools/pip_package/BUILD:179:10: Illegal ambiguous match on configurable attribute "data" in //tensorflow/tools/pip_package:licenses:
//third_party/mkl:build_with_mkl_aarch64
//tensorflow:arm_any

thus I removed it. May you please advise on the possible fix of "Illegal ambiguous match on configurable attribute"? The only new LICENSE file which this PR adds is in ./third_party/compute_library/, and I don't see from where this "ambiguous match" arises.

Have you tested with all targets that //tensorflow:arm_any covers?

The tests were performed with linux_aarch64 and arm64-v8a.

@alenik01
Copy link
Contributor Author

@penpornk, the new fix 37c1f29 actually excludes arm_any from if_mkl and mkl_deps.

Could you please help resolve merge conflicts again?

I am now building TF to test.

@google-cla
Copy link

google-cla bot commented Mar 24, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@alenik01
Copy link
Contributor Author

Could you please help resolve merge conflicts again?

Done.

Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

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

Thank you very much!

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 24, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 24, 2021
@agramesh1
Copy link
Contributor

agramesh1 commented Mar 24, 2021

Hi @penpornk sorry for the confusion. This commit 37c1f29 broke the oneDNN builds due to ambiguous definitions of
//third_party/mkl:build_with_mkl
//tensorflow:linux_x86_64

We verified that this change fixes it. Let me know if you can make the change or should we submit a PR. Thanks.

diff --git a/third_party/mkl/build_defs.bzl b/third_party/mkl/build_defs.bzl
index d913f22a110..7a4f08ed6a3 100644
--- a/third_party/mkl/build_defs.bzl
+++ b/third_party/mkl/build_defs.bzl
@@ -32,7 +32,6 @@ def if_mkl(if_true, if_false = []):
       may need it. It may be deleted in future with refactoring.
     """
     return select({
-        "@org_tensorflow//third_party/mkl:build_with_mkl": if_true,
         "//tensorflow:linux_x86_64": if_true,
         "//tensorflow:windows": if_true,
         "//conditions:default": if_false,
@@ -101,7 +100,6 @@ def mkl_deps():
       inclusion in the deps attribute of rules.
     """
     return select({
-        "@org_tensorflow//third_party/mkl:build_with_mkl": ["@mkl_dnn_v1//:mkl_dnn"],
         "@org_tensorflow//third_party/mkl:build_with_mkl_aarch64": ["@mkl_dnn_v1//:mkl_dnn_aarch64"],
         "//tensorflow:linux_x86_64": ["@mkl_dnn_v1//:mkl_dnn"],
         "//tensorflow:windows": ["@mkl_dnn_v1//:mkl_dnn"],

@penpornk
Copy link
Member

@agramesh1 Thank you for the patch! I'll make the change.

@googlebot I've verified that all the commits are from the same github user. Manually setting CLA to yes.

@penpornk penpornk added cla: yes and removed cla: no labels Mar 24, 2021
@google-cla
Copy link

google-cla bot commented Mar 24, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Mar 24, 2021
@penpornk penpornk added cla: yes and removed cla: no labels Mar 24, 2021
@penpornk
Copy link
Member

The fix is in. 5eabb16

No need to resolve merge conflicts again. I've already pulled this PR in and running tests internally.

@copybara-service copybara-service bot merged commit 9e26f3b into tensorflow:master Mar 25, 2021
PR Queue automation moved this from Approved by Reviewer to Merged Mar 25, 2021
@alenik01 alenik01 deleted the aarch64_build_patch branch March 25, 2021 19:26
@gbaned gbaned removed the stat:awaiting response Status - Awaiting response from author label Mar 26, 2021
@cfRod
Copy link
Contributor

cfRod commented Mar 26, 2021

Thanks everyone for sorting this out!

cfRod added a commit to cfRod/tensorflow that referenced this pull request Sep 15, 2021
… backend

Related to issue tensorflow#47415 and PR tensorflow#47775. Adding support for caching inner product primitives.
Includes patch file for oneDNN to include inner product, eltwise primitives and updates to ACL thread binding.
cfRod added a commit to cfRod/tensorflow that referenced this pull request Oct 5, 2021
Related to issue tensorflow#47415 and PR tensorflow#47775. Adding support for caching matmul primitives.
Updates onednn_acl_primitives.patch to include matmul primitives.
cfRod added a commit to cfRod/tensorflow that referenced this pull request Oct 7, 2021
Related to issue tensorflow#47415 and PR tensorflow#47775. Adding support for caching matmul primitives.
Updates onednn_acl_primitives.patch to include matmul primitives.
penpornk pushed a commit to penpornk/tensorflow that referenced this pull request Oct 11, 2021
Related to issue tensorflow#47415 and PR tensorflow#47775. Adding support for caching matmul primitives.
Updates onednn_acl_primitives.patch to include matmul primitives.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:mkl MKL related issues ready to pull PR ready for merge process size:L CL Change Size: Large
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

8 participants