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.

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

CMake: Drop support for OPT_FLAGS #13

Merged
merged 3 commits into from
Oct 16, 2017

Conversation

chfast
Copy link
Contributor

@chfast chfast commented Oct 16, 2017

  1. Adding -march=native flag by default is bad idea, because this makes the library not portable.
  2. If you want to optimize with -march=native just do
cmake -DCMAKE_CXX_FLAGS='-march=native -mtune=native'

1. Adding -march=native flag by default is bad idea, because this makes the library not portable.
2. If you want to optimize with -march=native just do

    cmake -DCMAKE_CXX_FLAGS='-march=native -mtune=native'
@chfast
Copy link
Contributor Author

chfast commented Oct 16, 2017

@howardwu can you review this?

@howardwu
Copy link
Member

Agreed, using -march=native -mtune=native affects portability for older CPUs.

Keeping the OPT_FLAGS is useful for preserving optimizations. How about using -g -O2 as default instead?

@chfast
Copy link
Contributor Author

chfast commented Oct 16, 2017

Using CMAKE_BUILD_TYPE=RelWithDebInfo will give you exactly -g -O2.

@chfast
Copy link
Contributor Author

chfast commented Oct 16, 2017

I usually build it with CMAKE_BUILD_TYPE=Release what is -O3 -DNDEBUG. No debug symbols.
I can set the default build type to RelWithDebInfo. We can also tweak flags for each build type. See https://cmake.org/cmake/help/v3.10/variable/CMAKE_LANG_FLAGS_RELWITHDEBINFO.html.

@howardwu
Copy link
Member

Setting CMAKE_BUILD_TYPE to RelWithDebInfo in CMakeLists seems fine.

@chfast
Copy link
Contributor Author

chfast commented Oct 16, 2017

Done.

@howardwu
Copy link
Member

LGTM

We should propagate this change up to libfqfft and libsnark, updating the respective READMEs.

@howardwu howardwu merged commit 03b719a into scipr-lab:master Oct 16, 2017
@chfast chfast deleted the cmake-drop-opt-flags branch October 16, 2017 10:56
@tromer
Copy link
Member

tromer commented Oct 16, 2017

I don't mind changing the interface to whatever is the "proper" CMake way du jour, but I suggest to keep the -march=native -mtune=native default behavior, for several reasons:

  1. Compatibility with existing 3rd-party build scripts.
  2. People that use libsnark for ad-hoc tests and benchmarks should get the fastest performance by default. (Experience shows that when people do naive benchmarks, and especially when they write other systems and compare it to an existing one line libsnark, they will not bother optimizing things.)
  3. Conversely, people that use libsnark for release-level stuff will actually read the README and are more likely to make a conscious decision.

... unless the improvement due to -march=native -mtune=native is negligible on all known platforms.

(Sorry for not seeing this pull request earlier. Something's broken with Github notifications for new pull requests, I'm getting notifications only on discussions of existing requests)

@tromer
Copy link
Member

tromer commented Oct 16, 2017

Also, the README (of libff and the other libs) should helpfully offer the magical incantations, known to few mortals, like cmake CMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_CXX_FLAGS='-march=native -mtune=native' .

@howardwu
Copy link
Member

@tromer How about checking for march support, e.g.

include(CheckCXXCompilerFlag)
CHECK_CXX_COMPILER_FLAG("-march=native" COMPILER_SUPPORTS_MARCH_NATIVE)
if(COMPILER_SUPPORTS_MARCH_NATIVE)
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -march=native -mtune=native")
endif()

@tromer
Copy link
Member

tromer commented Oct 16, 2017

@howardwu How does that help? AFAIK gcc always supports -march=native but the question is whether it should be used (which depends on whether you're going to run the executable on the local machine, as is common during development/benchmarking, or are building a version for distribution).

mobileink added a commit to minatools/libff that referenced this pull request Jul 20, 2020
# This is the 1st commit message:

bazel support, initial commit

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#2:

gitignore .bazelrc, bazel-*

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#3:

remove sha256 from rule_foreign_cc rule

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#4:

rename target ff to libff

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#5:

put curves/mnt/mnt4, 6 in separate packages

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#6:

bn128: drop "depends" from include prefix, for bazel compatibility

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#7:

fix refs to targets mnt4, mnt6, libff

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#8:

fix refs to @ate_pairing//:libgmp

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#9:

change @// to //

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#10:

delete obsolete mnt4, mnt6 targets from curves/mnt/BUILD.bazel

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#11:

add target scalar_multiplication:multiexp_profile

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#12:

list headers explicitly

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#13:

dead code elim

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#14:

dead code elim

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#15:

BUILD files: explicitate srcs/hdrs, DCE, buildifier reformat

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#16:

change @ate_pairing//:zm to @ate_pairing//ate-pairing

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#17:

switch obazl repos from local to git

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#18:

pin xbyak, ate-pairing repos to versions, to match upstream

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#19:

delete git submodules, not needed with Bazel

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#20:

add sha256 for xbyak, ate-pairing external repos

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#21:

remove xbyak dep - it's included in ate-pairing

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>

# This is the commit message scipr-lab#22:

restore depends dirs

Signed-off-by: Gregg Reynolds <601396+mobileink@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants