Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Update mkldnn window build instructions in MKLDNN_README.md #14952

Merged
merged 17 commits into from
May 19, 2019
Merged

Update mkldnn window build instructions in MKLDNN_README.md #14952

merged 17 commits into from
May 19, 2019

Conversation

yinghu5
Copy link
Contributor

@yinghu5 yinghu5 commented May 15, 2019

Description

Updates mkldnn & MKL window Build instructions in MKLDNN_README.md, if the PR #14877 (fix MKLDNN/MKL in cmake) is merged.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Feature1, tests, (and when applicable, API doc)
  • Feature2, tests, (and when applicable, API doc)

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@yinghu5 yinghu5 requested a review from szha as a code owner May 15, 2019 01:17
@yinghu5
Copy link
Contributor Author

yinghu5 commented May 15, 2019

@TaoLv @szha @ZhennanQin @pengzhao-intel please help to review them. thanks

@pengzhao-intel
Copy link
Contributor

@aaronmarkham could you help take a review for the document change? thanks in advance :)

aaronmarkham
aaronmarkham reviewed May 16, 2019
View reviewed changes
Copy link
Contributor

@aaronmarkham aaronmarkham left a comment

Choose a reason for hiding this comment

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

Is Visual Studio 2017 no longer compatible? I recall having both 2015 and 2017 in there so more people could use it with their preferred dev environment.

@yinghu5
Copy link
Contributor Author

yinghu5 commented May 17, 2019

@aaronmarkham thank you for the comment!. As the build steps in Visual Studio 2017 and visual studio 2015 is exact same, so i remove the visual studio 2017 part in second commit. The current version is like

Windows
On Windows, you can use Micrsoft Visual Studio 2015 and Microsoft Visual Studio 2017 to compile MXNet with Intel MKL-DNN. Micrsoft Visual Studio 2015 is recommended.

Visual Studio 2015 ...

@pengzhao-intel pengzhao-intel mentioned this pull request May 17, 2019
Closed
pengzhao-intel
pengzhao-intel reviewed May 17, 2019
View reviewed changes
Copy link
Contributor

@pengzhao-intel pengzhao-intel left a comment

Choose a reason for hiding this comment

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

I see there're several hardcode for the software versions. The advantage is the fix id can make the build more easily. On the other hand, the document should be a little flexible and maybe don't need to pin on specific version id.

How about giving a list of SW version in the document and mentioned which are fully tested version?

docs/tutorials/mkldnn/MKLDNN_README.md Outdated Show resolved Hide resolved
docs/tutorials/mkldnn/MKLDNN_README.md Outdated Show resolved Hide resolved
docs/tutorials/mkldnn/MKLDNN_README.md Outdated
<!--- KIND, either express or implied. See the License for the -->
<!--- specific language governing permissions and limitations -->
<!--- under the License. -->
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the change in the License part?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems the original EOL have LF and CRLF as file end flag. I change all of them to LF. thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see there're several hardcode for the software versions. The advantage is the fix id can make the build more easily. On the other hand, the document should be a little flexible and maybe don't need to pin on specific version id.

How about giving a list of SW version in the document and mentioned which are fully tested version?

The numpy etc, are common support library, version-insensitive, I removed the version information. thanks

pengzhao-intel
pengzhao-intel approved these changes May 17, 2019
View reviewed changes
Copy link
Contributor

@pengzhao-intel pengzhao-intel left a comment

Choose a reason for hiding this comment

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

LGTM

TaoLv
TaoLv approved these changes May 17, 2019
View reviewed changes
Copy link
Member

@TaoLv TaoLv left a comment

Choose a reason for hiding this comment

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

Overall it looks good to me. Just one minor comment.

docs/tutorials/mkldnn/MKLDNN_README.md
>cd build
>cmake -G "Visual Studio 14 Win64" .. -DUSE_CUDA=0 -DUSE_CUDNN=0 -DUSE_NVRTC=0 -DUSE_OPENCV=1 -DUSE_OPENMP=1 -DUSE_PROFILER=1 -DUSE_BLAS=open -DUSE_LAPACK=1 -DUSE_DIST_KVSTORE=0 -DCUDA_ARCH_NAME=All -DUSE_MKLDNN=1 -DCMAKE_BUILD_TYPE=Release
```
3. Enable Intel MKL-DNN and Intel MKL as BLAS library by the command:
Copy link
Member

Choose a reason for hiding this comment

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

MKL-DNN is already enabled in the command above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Highlight the MKL-DNN in above command. thanks

@yinghu5
Copy link
Contributor Author

yinghu5 commented May 18, 2019

The original md file mix two kind of line end of flag: linux: LR and window: CRLF. most of them are CRLF, so change to CRLF.

@pengzhao-intel
Copy link
Contributor

Thanks for your contribution. Merging now :)

@pengzhao-intel pengzhao-intel merged commit bd44ff4 into apache:master May 19, 2019
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@yinghu5
Update mkldnn window build instructions in MKLDNN_README.md (apache#1…
996f321 
…4952)

* update mklndd.md

* Update MKLDNN_README.md

simplify the windows build instruction

* Update MKLDNN_README.md

simply steps.

* Update MKLDNN_README.md

Simply VS 2017 part

* modify vs2017

* Update MKLDNN_README.md

remove numpy hardcode

* LF replace

* -DUSE_MKLDNN=1

* Update MKLDNN_README.md

resolve the conflict

* quantization
@TaoLv TaoLv mentioned this pull request Jul 9, 2019
Closed
5 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pengzhao-intel pengzhao-intel pengzhao-intel approved these changes

@aaronmarkham aaronmarkham aaronmarkham left review comments

@TaoLv TaoLv TaoLv approved these changes

@szha szha Awaiting requested review from szha szha is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yinghu5 @pengzhao-intel @aaronmarkham @TaoLv