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

Compile MKLDNN in Mac. #10115

Merged
merged 3 commits into from
Mar 15, 2018
Merged

Compile MKLDNN in Mac. #10115

merged 3 commits into from
Mar 15, 2018

Conversation

zheng-da
Copy link
Contributor

Description

This is to make MKLDNN compiled in Mac correctly.

Checklist

Essentials

  • Passed code style checking (make lint)
  • 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
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@zheng-da zheng-da requested a review from szha as a code owner March 14, 2018 20:18
szha
szha reviewed Mar 14, 2018
View reviewed changes
prepare_mkldnn.sh Outdated
make -C $MKLDNN_BUILDDIR -j$(sysctl -n hw.ncpu) VERBOSE=1 >&2
else
make -C $MKLDNN_BUILDDIR VERBOSE=1 >&2
fi
Copy link
Member

@szha szha Mar 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe refactor the processor discovery to reduce duplicated make lines? I used this snippet for discovering number of processes elsewhere.

NUM_PROC=1
if [[ ! -z $(command -v nproc) ]]; then
    NUM_PROC=$(nproc)
elif [[ ! -z $(command -v sysctl) ]]; then
    NUM_PROC=$(sysctl -n hw.ncpu)
else
    >&2 echo "Can't discover number of cores."
fi

lebeg
lebeg reviewed Mar 14, 2018
View reviewed changes
prepare_mkldnn.sh Outdated
@@ -84,7 +84,13 @@ if [ ! -f "$MKLDNN_INSTALLDIR/lib/libmkldnn.so" ]; then
cd $MXNET_ROOTDIR
g++ --version >&2
cmake $MKLDNN_ROOTDIR -DCMAKE_INSTALL_PREFIX=$MKLDNN_INSTALLDIR -B$MKLDNN_BUILDDIR -DARCH_OPT_FLAGS="-mtune=generic" >&2
make -C $MKLDNN_BUILDDIR -j$(cat /proc/cpuinfo | grep processor | wc -l) VERBOSE=1 >&2
if [ $OSTYPE == "linux-gnu" ]; then
make -C $MKLDNN_BUILDDIR -j$(cat /proc/cpuinfo | grep processor | wc -l) VERBOSE=1 >&2
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 -j$(nproc) will work on most platforms

Copy link
Contributor Author

@zheng-da zheng-da Mar 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

make -j$(nproc) doesn't seem to work on Mac

$ make -j$(nproc)
-bash: nproc: command not found```

prepare_mkldnn.sh Outdated
elif [ $OSTYPE == "darwin16" ]; then
make -C $MKLDNN_BUILDDIR -j$(sysctl -n hw.ncpu) VERBOSE=1 >&2
else
make -C $MKLDNN_BUILDDIR VERBOSE=1 >&2
Copy link
Contributor

Choose a reason for hiding this comment

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

Which OS is the else branch? Why multiple cores can not be used?

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 only listed two popular OS. I don't know what other OS people use and what mechanism is provided for getting the number of cores automatically. We need to handle the default case.

@szha
Copy link
Member

szha commented Mar 14, 2018

Given that our CI doesn't have mac yet, please verify the new logic locally.

@zheng-da
Copy link
Contributor Author

I tested it. It works fine.

@szha
Copy link
Member

szha commented Mar 14, 2018

Does the current logic require re-building mkldnn at every make? Is caching the build artifact possible?

@zheng-da
Copy link
Contributor Author

Yes, it's weird. We don't need to rebuild mkldnn every time in Linux.

@szha
Copy link
Member

szha commented Mar 14, 2018

This needs to be fixed.

@zheng-da
Copy link
Contributor Author

it should work now.

@pengzhao-intel
Copy link
Contributor

FYI, as @KellenSunderland @marcoabreu 's suggestion, we added Clang + MKLDNN instance in CI process #9918 (#9828) to track the OSX building issues.

@szha
Copy link
Member

szha commented Mar 15, 2018

clang build is different from building on mac. For example, clang build won't catch the processor detection problem.

@szha szha merged commit f7fa49e into apache:master Mar 15, 2018
@zheng-da zheng-da deleted the mkldnn_mac branch March 24, 2018 05:53
jinhuang415 pushed a commit to jinhuang415/incubator-mxnet that referenced this pull request Mar 30, 2018
@zheng-da
Compile MKLDNN in Mac. (apache#10115)
31df0ec 
* Compile MKLDNN in Mac.

* A more general way of finding #cores.

* find the right dyn library of mkldnn.
rahul003 pushed a commit to rahul003/mxnet that referenced this pull request Jun 4, 2018
@zheng-da @szha
Compile MKLDNN in Mac. (apache#10115)
6cd05c2 
* Compile MKLDNN in Mac.

* A more general way of finding #cores.

* find the right dyn library of mkldnn.
zheng-da added a commit to zheng-da/incubator-mxnet that referenced this pull request Jun 28, 2018
@zheng-da
Compile MKLDNN in Mac. (apache#10115)
5e75d18 
* Compile MKLDNN in Mac.

* A more general way of finding #cores.

* find the right dyn library of mkldnn.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lebeg lebeg lebeg left review comments

@szha szha szha left review comments

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
@zheng-da @szha @pengzhao-intel @lebeg