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

[CMAKE][ARM] USE_SSE option to govern USE_SSE in mshadow and -msse2 compil… #8395

Closed
wants to merge 1 commit into from
Closed

[CMAKE][ARM] USE_SSE option to govern USE_SSE in mshadow and -msse2 compil… #8395

wants to merge 1 commit into from

Conversation

larroy
Copy link
Contributor

@larroy larroy commented Oct 23, 2017

…er flags

Updated mshadow.

Description

This fixes Cmake build on ARM. (NO SSE support nor X86 specific includes like emmintrin.h)

@larroy larroy changed the title [CMAKE] USE_SSE option to govern USE_SSE in mshadow and -msse2 compil… [CMAKE][ARM] USE_SSE option to govern USE_SSE in mshadow and -msse2 compil… Oct 23, 2017
@cjolivier01
Copy link
Member

Just curious what the use-case is for not wanting sse

@cjolivier01
Copy link
Member

Doesn’t the SUPPORT_SSE test fail for platform which doesn’t support it?

@larroy
Copy link
Contributor Author

larroy commented Oct 23, 2017

non intel processors. Mshadow fails to compile since it includes a specific i386 header when sse is enabled.

No point in checking if you have it disabled. Just for completeness, since we have to expose the flag anyways.

@larroy
Copy link
Contributor Author

larroy commented Oct 23, 2017

dmlc/mshadow#302

@cjolivier01
Copy link
Member

Still not sure I understand -- currently it has an automatic detection system via checking for compiler support for that platform. Why are we also adding a manual switch? Is there a use-case where the compiler would pass the check-for-sse-support test but we wouldn't want ot use SSE anyway?

@cjolivier01
Copy link
Member

Also, what about visual C++? /arch:SSE flag? Is this currently supported? Will that flag disable it?

@larroy
Copy link
Contributor Author

larroy commented Oct 23, 2017

Wheres the visual C++ flag set?

@larroy
Copy link
Contributor Author

larroy commented Oct 23, 2017

I added the switch for consistency. I think it would be better to change this sse to -march=native eventually. Do you propose using SUPPORT_MSSE2 in ARM instead? the previous PR is already merged, and depends on USE_SSE flag.

@larroy
Copy link
Contributor Author

larroy commented Oct 23, 2017

Can you be more specific in what's your alternative proposal?

@cjolivier01
Copy link
Member

I just think it doesn't make logical sense. If @piiswrong likes it, then I am fine, but why t bothers me is:

  1. if SSE is supported, there's no reason not to use it, right? Why would anyone set USE_SSE to off?
  2. SUPPORTS_SSE check is automatic. Why expose to the user/developer?
  3. The check will still occur, so USE_SSE doesn't seem to actually say whether SSE will be used. Settin gthe flag is superfluous, and per Add some ops #1, you'd never turn it off manually anyway
  4. Other PR can be changed. Why can;t it check the compiler flag in mshadow so that it doesn't require mshadow consumers to know that they must set this flag?

@larroy
Copy link
Contributor Author

larroy commented Oct 24, 2017

Ok, you convinced me. I will close this PR and fix mshadow to use SUPPORTS_SSE. I think it's better not to expose this additional flag as you say. Thanks for your review.

@larroy larroy closed this Oct 24, 2017
@larroy larroy deleted the arm_build branch November 2, 2017 15:59
joseph-wakeling-sociomantic added a commit to joseph-wakeling-sociomantic/mxnet that referenced this pull request Jan 15, 2018
@joseph-wakeling-sociomantic
Merge stable 1.0.0 release of MXNet
e5e1064 
* cub 89de7ab2(89de7ab2)...05eb57fa(05eb57fa) (798 commits)
  > Merge pull request sociomantic-tsunami#1 from ptrendx/update
  > Create README.md
  > update readme.md
  > 1.7.0
  < 1.6.4 doc update (part 2)
  (...)

* dlpack ()...a6e09b5(a6e09b5) (1 commits)
  > Change order of device_type/id in Context (sociomantic-tsunami#11)

* dmlc-core a6c5701(a6c5701)...87b7ffa(87b7ffa) (54 commits)
  > add SetEnv (apache#322)
  > Fix a bug in seek/tell on Windows (apache#318)
  > Fixes apache#303: added recurse_directories to InputSplit::Create (apache#310)
  > Type name error (apache#316)
  > Small param bug (apache#315)
  (...)

* mshadow c037b06(c037b06)...2d7780c(2d7780c) (42 commits)
  > [CMAKE][ARM] Change USE_SSE to SUPPORT_MSSE2 to it uses the autodetected presence of sse compiler flag from the parent project (see PR apache#8395) (apache#303)
  > Makes repeated setting of gpu rng seed produce repeatable sequences. (apache#304)
  > Add USE_SSE which propagates into MSHADOW_USE_SSE in cmake (apache#302)
  > fix range (apache#301)
  > fix for random seed generation (apache#300)
  (...)

* nnvm b279286(b279286)...e4a138a(e4a138a) (139 commits)
  > [TVM] upgrade to latest version (apache#263)
  > Added support for CoreML Permute layers (apache#262)
  > [CMPL] Add Support for Other Data Types (apache#252)
  > fix onnx conv2d_transpose loading (apache#245)
  > [FIX] Fix from_mxnet for multiple outputs symbol (apache#247)
  (...)

* ps-lite v1+118(acdb698)...v1+123(2ce8b9a) (2 commits)
  > Merge pull request apache#117 from madjam/listen-interface
  > Merge pull request apache#109 from b0noI/master
joseph-wakeling-sociomantic added a commit to joseph-wakeling-sociomantic/mxnet that referenced this pull request Jan 15, 2018
@joseph-wakeling-sociomantic
Merge stable 1.0.0 release of MXNet
2f3fc17 
Fixes sociomantic-tsunami#11

* cub 89de7ab2(89de7ab2)...05eb57fa(05eb57fa) (798 commits)
  > Merge pull request sociomantic-tsunami#1 from ptrendx/update
  > Create README.md
  > update readme.md
  > 1.7.0
  < 1.6.4 doc update (part 2)
  (...)

* dlpack ()...a6e09b5(a6e09b5) (1 commits)
  > Change order of device_type/id in Context (sociomantic-tsunami#11)

* cub 89de7ab2(89de7ab2)...05eb57fa(05eb57fa) (798 commits)
  > Merge pull request sociomantic-tsunami#1 from ptrendx/update
  > Create README.md
  > update readme.md
  > 1.7.0
  < 1.6.4 doc update (part 2)
  (...)

* dlpack ()...a6e09b5(a6e09b5) (1 commits)
  > Change order of device_type/id in Context (sociomantic-tsunami#11)

* dmlc-core a6c5701(a6c5701)...87b7ffa(87b7ffa) (54 commits)
  > add SetEnv (apache#322)
  > Fix a bug in seek/tell on Windows (apache#318)
  > Fixes apache#303: added recurse_directories to InputSplit::Create (apache#310)
  > Type name error (apache#316)
  > Small param bug (apache#315)
  (...)

* mshadow c037b06(c037b06)...2d7780c(2d7780c) (42 commits)
  > [CMAKE][ARM] Change USE_SSE to SUPPORT_MSSE2 to it uses the autodetected presence of sse compiler flag from the parent project (see PR apache#8395) (apache#303)
  > Makes repeated setting of gpu rng seed produce repeatable sequences. (apache#304)
  > Add USE_SSE which propagates into MSHADOW_USE_SSE in cmake (apache#302)
  > fix range (apache#301)
  > fix for random seed generation (apache#300)
  (...)

* nnvm b279286(b279286)...e4a138a(e4a138a) (139 commits)
  > [TVM] upgrade to latest version (apache#263)
  > Added support for CoreML Permute layers (apache#262)
  > [CMPL] Add Support for Other Data Types (apache#252)
  > fix onnx conv2d_transpose loading (apache#245)
  > [FIX] Fix from_mxnet for multiple outputs symbol (apache#247)
  (...)

* ps-lite v1+118(acdb698)...v1+123(2ce8b9a) (2 commits)
  > Merge pull request apache#117 from madjam/listen-interface
  > Merge pull request apache#109 from b0noI/master
@joseph-wakeling-sociomantic joseph-wakeling-sociomantic mentioned this pull request Jan 15, 2018
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cjolivier01 cjolivier01 Awaiting requested review from cjolivier01

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

Successfully merging this pull request may close these issues.
2 participants
@larroy @cjolivier01