[MXNET-372] Add build flag for USE_F16C in CMake and clarify flag in make #10760

rahul003 · 2018-05-01T01:07:23Z

Description

Added build flag for USE_F16C in CMake because the CMake for mshadow is not included in certain cmake code paths
Added the same flag in config.mk

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 http://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

marcoabreu · 2018-05-01T01:16:51Z

CMakeLists.txt

@@ -102,6 +103,10 @@ else(MSVC)
  else()
    set(SUPPORT_MSSE2 FALSE)
  endif()
+  # If USE_F16C set to ON, mshadow checks support
+  if(NOT USE_F16C)
+    set(SUPPORT_MF16C FALSE)


Please print a message if this is getting disabled

marcoabreu · 2018-05-01T01:17:07Z

make/config.mk

@@ -132,10 +132,19 @@ endif
 ARCH := $(shell uname -a)
 ifneq (,$(filter $(ARCH), armv6l armv7l powerpc64le ppc64le aarch64))
 	USE_SSE=0
+	USE_F16C=0


Please print a message if this is getting disabled

That's not the behavior of any make flag though. Cmake does do that. Also for those devices there, F16C instruction set is absent. So it's not really a choice

Well this is basically the first case, right? But I guess you're right, so it's fine that way.

marcoabreu · 2018-05-01T02:03:30Z

CMakeLists.txt

@@ -102,6 +104,29 @@ else(MSVC)
  else()
    set(SUPPORT_MSSE2 FALSE)
  endif()
+  # For cross complication, turn off flag if target device does not support it
+  if(USE_F16C AND NOT MSVC)


It is already guaranteed that MSVC is not present at this point

Yeah corrected it thanks

marcoabreu · 2018-05-01T02:26:57Z

CMakeLists.txt

@@ -86,6 +87,8 @@ if(MSVC)
  add_definitions(-DNNVM_EXPORTS)
  add_definitions(-DDMLC_STRICT_CXX11)
  add_definitions(-DNOMINMAX)
+  set(SUPPORT_F16C FALSE)


This question might be stupid, but why are we disabling support on Windows? If this is a processor instruction, why does it depend on the OS?

It's actually the MSVC (visual studio compiler) that's not yet supported. That doesn't support the current usage of f16c functions. One of the problems being that it does not have any specific link flag for f16c and the header file used does not exist. Future work, I'll see whether there are analogous features. Updated the message.
To use the feature we need not just processor, but also compiler support.

Aaah I see, that makes sense. Thanks for elaborating

anirudh2290 · 2018-05-01T04:28:11Z

make/config.mk

+# For distributed training with fp16, this helps even if training on GPUs
+# If left empty, checks CPU support and turns it on.
+# For cross compilation, please check support for F16C on target device and turn off if necessary.
+USE_F16C =


Why aren't we adding similar logic like CMake for F16C support to Makefile ?

That is in mshadow's makefile. I didn't add it here because it is always included in all code paths.

IMO, we should try to do most work (including whether hardware has FP16 support, and compiler has Fp16 support) in MXNet itself and pass the flag to Mshadow, so that we reduce the dependency on mshadow. This way MXNet users won't be impacted if something in MShadow breaks. WDYT ?

I think this is hard to pull off, considering it's a compile flag, and overcomplicates our build-process even further. As @hen pointed out on dev@, we should rather find a more permanent solution rather than trying to add even more hacks

@rahul003 @marcoabreu I am okay with leaving this for now if its too much effort. Having said that, I think this stop-gap solution is better than what we have today if it means less pain for the user until we move to the more permanent solution. Considering that permanent solution (or atleast one of them) would be to move all the dmlc projects under apache (so that MXNet has better control on the repos), I don't see it happening anytime soon.

I've added the logic of identifying the flag here in MXNet. MShadow's role now is to use that flag if defined and set the compiler flag. This means we have control over when to turn on or off the flag in MXNet

marcoabreu · 2018-05-01T04:45:33Z

CMakeLists.txt

+              COMMAND grep F16C
+              OUTPUT_VARIABLE CPU_SUPPORT_F16C)
+    endif()
+    if(NOT CPU_SUPPORT_F16C)


Is an unitiliazed variable automatically assumed as FALSE?

We never set SUPPORT_MF16C explicitely to FALSE

Yes that's right

…to f16c-build

…16c-build

…make (apache#10760) * add comments to makefile config * add f16c check in mxnet * update cmake * clarify * small updates * add message * add message * update msvc message * update mshadow * typo * only print message for MSVC if USE_F16C * improve build logic * update mshadow * remove def from amalgamation makefile * trigger CI

add comments to makefile config

92ce673

rahul003 requested a review from szha as a code owner May 1, 2018 01:07

marcoabreu reviewed May 1, 2018

View reviewed changes

rahul003 added 2 commits April 30, 2018 18:33

add f16c check in mxnet

4d5a745

update cmake

93df4fc

marcoabreu reviewed May 1, 2018

View reviewed changes

rahul003 added 5 commits May 1, 2018 02:08

clarify

d00eaa2

small updates

04dc1ea

small updates:

56fe747

add message

3997985

add message

9c9b002

apache deleted a comment from rahul003 May 1, 2018

marcoabreu reviewed May 1, 2018

View reviewed changes

update msvc message

9407ec4

anirudh2290 reviewed May 1, 2018

View reviewed changes

marcoabreu reviewed May 1, 2018

View reviewed changes

anirudh2290 approved these changes May 1, 2018

View reviewed changes

rahul003 added 3 commits May 1, 2018 18:09

update mshadow

e50bc28

Merge branch 'master' of https://github.com/apache/incubator-mxnet in…

b1a8477

…to f16c-build

Merge branch 'f16c-build' of https://github.com/rahul003/mxnet into f…

62b01bd

…16c-build

rahul003 force-pushed the f16c-build branch from 62b01bd to e50bc28 Compare May 1, 2018 20:57

rahul003 requested a review from yzhliu as a code owner May 1, 2018 21:05

rahul003 force-pushed the f16c-build branch from 9b577c2 to 62b01bd Compare May 1, 2018 21:06

This was referenced May 1, 2018

[PR for v1.2] F16C Build flags #10769

Closed

[MXNET-372] Add build flag for USE_F16C in CMake and clarify flag in make #10771

Merged

rahul003 added 3 commits May 1, 2018 21:48

typo

806b500

only print message for MSVC if USE_F16C

92a968d

improve build logic

d52b56d

rahul003 mentioned this pull request May 1, 2018

NNVM build failed in the newst mxnet version #10558

Closed

rahul003 added 4 commits May 1, 2018 23:30

update mshadow

a5d51c7

remove def from amalgamation makefile

b3cc976

update mshadow

0557db7

trigger CI

ca83edb

anirudh2290 merged commit 1822dac into apache:master May 2, 2018

rahul003 deleted the f16c-build branch May 8, 2018 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MXNET-372] Add build flag for USE_F16C in CMake and clarify flag in make #10760

[MXNET-372] Add build flag for USE_F16C in CMake and clarify flag in make #10760

rahul003 commented May 1, 2018 •

edited

Loading

marcoabreu May 1, 2018

marcoabreu May 1, 2018

rahul003 May 1, 2018

marcoabreu May 1, 2018 •

edited

Loading

marcoabreu May 1, 2018

rahul003 May 1, 2018

marcoabreu May 1, 2018

rahul003 May 1, 2018 •

edited

Loading

marcoabreu May 1, 2018

anirudh2290 May 1, 2018 •

edited

Loading

rahul003 May 1, 2018

anirudh2290 May 1, 2018

marcoabreu May 1, 2018

anirudh2290 May 1, 2018

rahul003 May 1, 2018

marcoabreu May 1, 2018

marcoabreu May 1, 2018

rahul003 May 1, 2018

[MXNET-372] Add build flag for USE_F16C in CMake and clarify flag in make #10760

[MXNET-372] Add build flag for USE_F16C in CMake and clarify flag in make #10760

Conversation

rahul003 commented May 1, 2018 • edited Loading

Description

Checklist

Essentials

Changes

Comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoabreu May 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahul003 May 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anirudh2290 May 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahul003 commented May 1, 2018 •

edited

Loading

marcoabreu May 1, 2018 •

edited

Loading

rahul003 May 1, 2018 •

edited

Loading

anirudh2290 May 1, 2018 •

edited

Loading