Compiler::openmp_flag() by davydden · Pull Request #882 · spack/spack

davydden · 2016-05-03T09:10:12Z

fixes #876

alalazo · 2016-05-03T09:13:04Z

lib/spack/spack/compilers/intel.py

                   'fc'  : 'intel/ifort' }

+    @property
+    def openmp_flag(self):


I think it's -qopenmp in the latest version.

do you know from which version?

here they say -openmp for 15.0 : https://software.intel.com/en-us/node/522690

I can't find a web page showcasing it, but the change should start from 16:

$ icc --version icc (ICC) 16.0.2 20160204 Copyright (C) 1985-2016 Intel Corporation. All rights reserved. $ icc -openmp main.c icc: command line remark #10411: option '-openmp' is deprecated and will be removed in a future release. Please use the replacement option '-qopenmp'

ok, i will add a check for 16.0...

davydden · 2016-05-03T09:42:55Z

there is a fancy issue. I am testing the two methods by calling them in package

    def install(self, spec, prefix):
        cxx11_compiler_flag  = self.compiler.cxx11_flag()
        openmp_compiler_flag = self.compiler.openmp_flag()

which ends up with an error

==> Building openblas
Traceback (most recent call last):
  File "/Users/davydden/spack/lib/spack/spack/build_environment.py", line 367, in fork
    function()
  File "/Users/davydden/spack/lib/spack/spack/package.py", line 913, in build_process
    self.install(self.spec, self.prefix)
  File "/Users/davydden/spack/var/spack/repos/builtin/packages/openblas/package.py", line 25, in install
    cxx11_compiler_flag  = self.compiler.cxx11_flag()
TypeError: 'str' object is not callable
==> Error: Installation process had nonzero exit code.

Interestingly enough this

def install(self, spec, prefix):
        # cxx11_compiler_flag  = self.compiler.cxx11_flag()
        openmp_compiler_flag = self.compiler.openmp_flag()

works just fine. I don't understand how this is possible. Any advices from the python gurus 😄 ?

alalazo · 2016-05-03T09:55:46Z

Take a look at the @property decorator... 😄

You don't need the function call syntax to execute that code : from the client point of view it will look like accessing an attribute. What happens in your error is that you got a string returned with :

self.compiler.cxx11_flag

and then you try to __call__ the string, hence:

TypeError: 'str' object is not callable

davydden · 2016-05-03T10:03:52Z

@alalazo thanks for explanations 👍 I learned something new today 😄

davydden · 2016-05-03T10:06:55Z

Ok, i checked both functions and they work as expected for Apple's clang and gcc. So it should be good as far as I can see.

davydden · 2016-05-03T13:10:03Z

@alalazo i deleted the last comment as I found out this is not the case.

adamjstewart · 2016-05-03T14:33:39Z

lib/spack/spack/compilers/clang.py

+        if ver_string.endswith('-apple'):
+            return True
+        else:
+            return False


I think a more Pythonic implementation would be:

return ver_string.endswith('-apple')

Actually, this would be even better:

@property def is_apple(self): ver_string = str(self.version) return ver_string.endswith('-apple')

will do, looks much better!

adamjstewart · 2016-05-03T14:52:09Z

Wow, this looks way better than my implementation. Can you add a default openmp_flag() and cxx11_flag() to the Compiler class in lib/spack/spack/compiler.py? They can either return a default string, or just return an error message saying that this function hasn't been implemented yet for this particular compiler. Then, each compiler will override it as necessary. This should handle compilers that don't implement cxx11_flag().

adamjstewart · 2016-05-03T15:16:24Z

lib/spack/spack/compilers/pgi.py

+    @property
+    def cxx11_flag(self):
+        tty.die("cxx11_flag() is not implemented for pgi. Consider creating a pull-request.")
+        return "-std=c++11"


From the man page for pgc++, it appears that PGI accepts either -std=c++11 or --c++11 so you can remove this message.

davydden · 2016-05-03T15:28:51Z

@adamjstewart i (hopefully) addressed you comments.

davydden · 2016-05-03T15:38:57Z

there is also

    # argument used to get C++11 options
    cxx11_flag = "-std=c++11"

in the Compiler.py. I think this should be removed as all dervied classes use @property instead anyway and now Compiler class additionally has a default implementation

    @property
    def cxx11_flag(self):
        return "-std=c++11"

adamjstewart · 2016-05-03T15:59:48Z

lib/spack/spack/compiler.py

+    # Override in derived classes if needed
+    @property
+    def cxx11_flag(self):
+        return "-std=c++11"


The more I think about it, these properties should probably look something like:

# This property should be overridden in the compiler subclass if # OpenMP is supported by that compiler @property def openmp_flag(self): # If it is not overridden, assume it is not supported and warn the user tty.die("The compiler you have chosen does not currently support OpenMP. If you think it should, please edit the compiler subclass and submit a pull request or issue.") # This property should be overridden in the compiler subclass if # C++11 is supported by that compiler @property def cxx11_flag(self): # If it is not overridden, assume it is not supported and warn the user tty.die("The compiler you have chosen does not currently support C++11. If you think it should, please edit the compiler subclass and submit a pull request or issue.")

The benefit of this is that if someone adds a new compiler but forgets to override these properties, or the compiler does not support them, Spack warns the user. Otherwise, the current implementation will just use the wrong flag and crash. Feel free to shorten up the message if you want.

sounds good to me, will change the behaviour.

adamjstewart · 2016-05-03T16:21:15Z

@davydden: Yes, I agree about removing:

# argument used to get C++11 options
cxx11_flag = "-std=c++11"

from compilers.py. Your property is replacing this variable.

…d on the compiler version

…pple or not using version

…properties are not implemented in a derived class

…y with the same name

…en in derived classes

davydden · 2016-05-05T08:51:50Z

@gartung since I substitute a cxx14_flag attributed introduced in #802 by a property with the same name, i though you might want give a quick look.

davydden · 2016-05-05T09:57:35Z

quickly documented the usage of compiler flags.

gartung · 2016-05-05T12:06:57Z

Submitted this a while back. Your feature is probably more correct. Feel free to remove my change if needed.

eschnett · 2016-05-05T13:18:30Z

I notice there are efforts to add OpenMP flags, C++14 flags, and names of executables such as mpicc. For consistency, they should all use the same kind of mechanism. I didn't check -- is this already the case?

davydden · 2016-05-05T13:22:19Z

no, they don't. Flags are done using @property which is overriden in derived clasess. Compiler flags are queried directly in a package. Whereas mpicc and alike are set in setup_dependent_package as they are used in packages which depend on them. This follows the logic introduced here #657 . I personally don't see a problem with that and don't think one has to use the same mechanism as the two are different.

tgamblin · 2016-05-09T19:42:56Z

lib/spack/spack/compiler.py

+    @property
+    def openmp_flag(self):
+        # If it is not overridden, assume it is not supported and warn the user
+        tty.die("The compiler you have chosen does not currently support OpenMP. If you think it should, please edit the compiler subclass and submit a pull request or issue.")


can these lines be made <80chars?

sure they can 😄 i am on it...

[Commercial] pull-in #375 with line length 80 chars ? 😄

@alalazo: good idea!

@alalazo: pretty sure your screen is at like 120 or something

tgamblin · 2016-05-09T19:44:43Z

@davydden @alalazo @adamjstewart @gartung thanks! this look great. I'll just shorten a few lines and merge.

tgamblin · 2016-05-09T19:45:25Z

@eschnett: if compilers are made into dependencies eventually these could potentially use similar mechanisms. Right now since they're special they're a little different.

tgamblin · 2016-05-09T19:46:37Z

@davydden: ok I'll let you shorten the lines :)

davydden · 2016-05-09T20:05:34Z

@tgamblin shortened 😄

davydden force-pushed the openmp_compiler_flag branch from 5053480 to 524230b Compare May 3, 2016 09:10

alalazo reviewed May 3, 2016
View reviewed changes

davydden force-pushed the openmp_compiler_flag branch from 355612e to bd850e4 Compare May 3, 2016 12:59

davydden changed the title ~~Compiler::openmp_flag() plus minor fixes~~ Compiler::openmp_flag() May 3, 2016

davydden force-pushed the openmp_compiler_flag branch 2 times, most recently from 3101db1 to 355612e Compare May 3, 2016 13:08

adamjstewart reviewed May 3, 2016
View reviewed changes

davydden mentioned this pull request May 4, 2016

make compilers a build dependency without using depends_on() #896

Closed

davydden added 8 commits May 5, 2016 10:44

compilers: make sure cxx11_flag() is defined for all compilers

2cdfe14

compilers: add openmp_flag() method

9776dc0

compilers: add missing import statements

c078dea

compilers: minor fixes to Clang::cxx11_flag() and Clang::openmp_flag()

3cd3052

compilers: make Intel::openmp_flag() return -openmp and -qopenmp base…

592045c

…d on the compiler version

compiler: add Clang.is_apple property which checks if Clang is from A…

07fd0cc

…pple or not using version

compilers: one more missing import statement

f2f1c49

compiler: simplify Clang.is_apple

6a418cf

davydden added 7 commits May 5, 2016 10:44

compiler: add default implementation of openmp_flag() and css11_flag()

d5a7607

compiler: cleanup Nag.cxx11_flag and Pgi.cxx11_flag

e28ca39

fix comment in Compiler class

30b65d3

compiler: make default openmp_flag() and cxx11_flag() die when these …

473a554

…properties are not implemented in a derived class

add a temporary Nag.cxx11_flag property

ddcb97f

remove cxx11_flag from Compiler as it is now substituted by a propert…

c6fb6bd

…y with the same name

substitute cxx14_flag by a property with the same name to be overridd…

f84f045

…en in derived classes

davydden force-pushed the openmp_compiler_flag branch from 6937081 to f84f045 Compare May 5, 2016 08:48

document usage of compiler flags properties

c37ea9a

This was referenced May 5, 2016

Allow compilers to specify their own rpath linking flags #905

Merged

openblas: do not use OpenMP with Clang #806

Merged

tgamblin reviewed May 9, 2016
View reviewed changes

wrap tty.die to 80 chars

7a2d659

tgamblin merged commit cbae986 into spack:develop May 9, 2016

adamjstewart mentioned this pull request May 10, 2016

Elk: Use new openmp_flag and mpi attributes #932

Merged

adamjstewart mentioned this pull request Nov 21, 2016

hdf5: added +fpic variant #2373

Merged

davydden deleted the openmp_compiler_flag branch May 28, 2017 20:57

olupton pushed a commit to olupton/spack that referenced this pull request Feb 7, 2022

morpho-kit: stop building python bindings for ++c (spack#882)

d11b9eb

Conversation

davydden commented May 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davydden May 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davydden commented May 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented May 3, 2016

Uh oh!

davydden commented May 3, 2016

Uh oh!

davydden commented May 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davydden commented May 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamjstewart commented May 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davydden commented May 3, 2016

Uh oh!

davydden commented May 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamjstewart commented May 3, 2016

Uh oh!

davydden commented May 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davydden commented May 5, 2016

Uh oh!

gartung commented May 5, 2016

Uh oh!

eschnett commented May 5, 2016

Uh oh!

davydden commented May 5, 2016

Uh oh!

tgamblin May 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgamblin commented May 9, 2016

Uh oh!

tgamblin commented May 9, 2016

Uh oh!

tgamblin commented May 9, 2016

Uh oh!

davydden commented May 9, 2016

Uh oh!

Reviewers

davydden May 3, 2016 •

edited

Loading

davydden commented May 3, 2016 •

edited

Loading

davydden commented May 3, 2016 •

edited

Loading

davydden commented May 3, 2016 •

edited

Loading

davydden commented May 5, 2016 •

edited

Loading

tgamblin May 9, 2016 •

edited

Loading