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

[INTEL MKL] Fix typo error of an environment variable #22324

Merged
merged 4 commits into from
Sep 28, 2018
Merged

[INTEL MKL] Fix typo error of an environment variable #22324

merged 4 commits into from
Sep 28, 2018

Conversation

gzmkl
Copy link
Contributor

@gzmkl gzmkl commented Sep 17, 2018

Change the name of an environment variable
from
TF_MKL_OPTIMIZE_PRIMITVE_MEMUSE
to
TF_MKL_OPTIMIZE_PRIMITIVE_MEMUSE

@qlzh727 qlzh727 self-assigned this Sep 18, 2018
@qlzh727 qlzh727 added the kokoro:force-run Tests on submitted change label Sep 18, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 18, 2018
@qlzh727 qlzh727 added the awaiting review Pull request awaiting review label Sep 18, 2018
@qlzh727
Copy link
Member

qlzh727 commented Sep 18, 2018

Can u fix the cl format lint error?

https://source.cloud.google.com/results/invocations/c3df27fd-ff97-4e5f-b9a6-8f309dcf940b/targets/%2F%2Ftensorflow%2Ftools%2Fci_build:gen_ci_clang_format_out/log

diff --git a/tensorflow/core/util/mkl_util.h b/tensorflow/core/util/mkl_util.h
index 5ea8f2ee47..06c3598f85 100644
--- a/tensorflow/core/util/mkl_util.h
+++ b/tensorflow/core/util/mkl_util.h
@@ -2041,7 +2041,7 @@ class MklPrimitiveFactory {
static inline bool IsPrimitiveMemOptEnabled() {
bool is_primitive_mem_opt_enabled = true;
TF_CHECK_OK(ReadBoolFromEnvVar("TF_MKL_OPTIMIZE_PRIMITIVE_MEMUSE", true,

  •      &is_primitive_mem_opt_enabled));

  •                               &is_primitive_mem_opt_enabled));
    return is_primitive_mem_opt_enabled;
    }

@gzmkl
Copy link
Contributor Author

gzmkl commented Sep 18, 2018

I fixed a couple of coding style issues by running cpplint.py utility.
But I am not sure if it addressed the issue that Qianli pointed out (not so obvious to me :)).
Thanks!

tatianashp
tatianashp reviewed Sep 20, 2018
View reviewed changes
tensorflow/core/util/mkl_util.h Outdated
@@ -2098,7 +2098,7 @@ static inline memory::format get_desired_format(int channel,
(channel % 8) == 0) {
fmt_desired = is_2d
? memory::format::nChw8c
: memory::format::ncdhw; //not support avx2 for 3d yet.
: memory::format::ncdhw; // not support avx2 for 3d yet.
Copy link
Member

Choose a reason for hiding this comment

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

Please change the comment to "no avx2 support for 3d yet"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tatianashp
tatianashp requested changes Sep 20, 2018
View reviewed changes
tensorflow/core/util/mkl_util.h Outdated
@@ -13,8 +13,8 @@ See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/

#ifndef TENSORFLOW_CORE_UTIL_MKL_UTIL_H_
#define TENSORFLOW_CORE_UTIL_MKL_UTIL_H_
#ifndef TENSORFLOW_TENSORFLOW_CORE_UTIL_MKL_UTIL_H_
Copy link
Member

Choose a reason for hiding this comment

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

This #ifndef was fine as it is. What is the purpose of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I ran cpplint.py and somehow the utility complains about it.

If it is fine, let me change it back, to make it consistent across all header files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done by changing back the macro def.

@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Sep 20, 2018
@tatianashp
Copy link
Member

tatianashp commented Sep 21, 2018 via email

@tatianashp tatianashp added the kokoro:force-run Tests on submitted change label Sep 21, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 21, 2018
@qlzh727 qlzh727 added the kokoro:force-run Tests on submitted change label Sep 24, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 24, 2018
@qlzh727 qlzh727 added the ready to pull PR ready for merge process label Sep 25, 2018
@gzmkl
Copy link
Contributor Author

gzmkl commented Sep 25, 2018

Hi Tatiana,
I noticed that this PR is "merge" blocked due to some CI build issue.
Please let me know what I need follow up.
Regards,
Guozhong

@tensorflow-copybara tensorflow-copybara merged commit 59a47b7 into tensorflow:master Sep 28, 2018
tensorflow-copybara pushed a commit that referenced this pull request Sep 28, 2018
@tensorflower-gardener
Merge pull request #22324 from Intel-tensorflow:fix_typo_env
66b0255 
PiperOrigin-RevId: 214983237
@claynerobison claynerobison deleted the fix_typo_env branch March 22, 2019 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tatianashp tatianashp tatianashp approved these changes

@rmlarsen rmlarsen Awaiting requested review from rmlarsen

Assignees

@qlzh727 qlzh727

Labels
cla: yes ready to pull PR ready for merge process
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@gzmkl @qlzh727 @tatianashp @googlebot @kokoro-team @tensorflowbutler @tensorflow-copybara