search

ROOTPWA-Maintainers / ROOTPWA

ROOTPWA is a toolkit for partial-wave analysis of multi-particle final states produced in high-energy particle reactions. It is used to determine hadron spectra from experimental data.
Other
8 stars 14 forks source link

[Bug] Compiler flags/options [sf#24] #24

Closed legordian closed 10 years ago

legordian commented 10 years ago

Reported by sebastianuhl on 2013-11-05 01:07 UTC Currently there seem to be a number of (smaller) issues with the compiler options.

1.) It is not possible to add any compiler flags from the command line via environment variables or defines, in CMakeLists.txt line 99 should be replaced by

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Woverloaded-virtual -Werror")

2.) The subdirectories pwa2000/gamp and pwa2000/libpp do not use the compiler flags used in the rest of the build. This is quite destructive if one for example has to compile a 32bit version on a system that by default builds 64bit. pwa2000/gamp/CMakeLists.txt and pwa2000/libpp/CMakeLists.txt should be changed in lines 38&39 (or probably it would be better to remove those lines):

set(CMAKE_CXX_FLAGS   "${CMAKE_CXX_FLAGS}   -O2 -Wall -Werror")
set(CMAKE_CXX_LDFLAGS "${CMAKE_CXX_LDFLAGS} -O2")

3.) By default all optimizations available on the computer the software is compiled on are enabled (due to the compiler option march=native). This is bad if the software is compiled at a rather new computer, but also has to work on older hardware which do not have the same set of instructions available. ROOTPWA might then crash with

 *** Break *** illegal instruction
legordian commented 10 years ago

Commented by legordian on 2013-11-26 16:59 UTC This is fixed in [e26ce4]. However, this commit currently resides on a development branch ("unstableIsobarAmplitudes") which will not be merged into master for some time. Maybe a cherry-pick might be the right option?

legordian commented 10 years ago

Commented by bgrube on 2014-01-15 15:05 UTC Cherry-picked changes from [e26ce4] into master branch. The -match-native switch is disabled since [20a8f5]. Please confirm that [f5ff2e] fixes the issues.

legordian commented 10 years ago

Commented by sebastianuhl on 2014-01-15 15:25 UTC 1.) As far as I understand CMake (but I have only found this documented on the level of experience, not by the vendor) defining

set(CMAKE_CXX_FLAGS_DEBUG "... ${CMAKE_CXX_FLAGS}")
set(CMAKE_CXX_FLAGS_RELEASE "... ${CMAKE_CXX_FLAGS}")

should not be necessary as the ${CMAKE_CXX_FLAGS} will be automatically appended.

2.) In the two pwa2000 directories I do not think the variable ${CMAKE_CXX_LDFLAGS} should be used, but ${CMAKE_CXX_FLAGS}.

legordian commented 10 years ago

Commented by bgrube on 2014-01-15 20:27 UTC Thanks for the info. Indeed cmake calls the compiler like

g++ ${CMAKE_CXX_FLAGS} ${CMAKE_CXX_FLAGS_RELEASE} ...

[5eaf36] should fix 1) and 2). I also swapped the order in which values from command line are applied to what was proposed in original bug report, that is:

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} <other flags>")

This way it is impossible to override the flags set in the CMakeLists.txt from the command line. Please confirm, that things work as expected. In particular it would be good to know, whether forcing 32bit compilation works now.

legordian commented 10 years ago

Commented by bgrube on 2014-01-16 18:28 UTC gcc 4.3 caused some extra headaches, but with [90924a] things should work now. Tests included 32bit compilation on 64bit SLC 5 machine at CERN. Please confirm.

legordian commented 10 years ago

Commented by sebastianuhl on 2014-01-16 18:58 UTC Compiles also for me.

One last question concerning the problem with gcc 4.3. Shouldn't it simply be possible to write

N += std::norm(_Vflat.amp())

std::norm should return a double, so this should also not affect the imaginary part.

legordian commented 10 years ago

Updated by sebastianuhl on 2014-01-16 18:59 UTC