Opened 6 years ago

Closed 6 years ago

#50958 closed defect (fixed)

py-graph-tool fails to build with clang OpenMP option (configure.args=--enable-openmp)

Reported by: essandess (Steve Smith) Owned by: count0 (Tiago de Paula Peixoto)
Priority: Normal Milestone:
Component: ports Version: 2.3.4
Keywords: Cc: mamoll (Mark Moll), setack@…, petrrr
Port: py-graph-tool

Description

Macports clang 3.7 and 3.8 now support OpenMP, which creates the opportunity to build graph-tool on OS X with OpenMP speeds.

I expected that the option configure.args=--enable-openmp would tell the autoconf'd ./configure command to specify --enable-openmp, but it does not:

# uninstall existing graph-tool
sudo port uninstall py-graph-tool py27-graph-tool
sudo port clean --dist py-graph-tool py27-graph-tool

# build new graph-tool with OpenMP support
sudo port install clang-3.8 +openmp
sudo port -d install py-graph-tool py27-graph-tool configure.compiler=macports-clang-3.8 configure.args=--enable-openmp

…
DEBUG: Executing command line: cd "/opt/local/var/macports/build/_opt_local_var_macports_sources_rsync.macports.org_release_tarballs_ports_python_py-graph-tool/py27-graph-tool/work/graph-tool-2.13" && ./configure --prefix=/opt/local --with-boost=/opt/local --exec-prefix=/opt/local/Library/Frameworks/Python.framework/Versions/2.7
…
checking whether to enable parallel algorithms with openmp... no

The port notes for libomp say:

Use with clang-3.[78] (when built with +openmp variant) by adding
"-fopenmp" to your compile and link lines.
(Or "-I/opt/local/include/libomp -L/opt/local/lib/libomp -fopenmp"
if clang is installed without +openmp.)

Based on the Port Phases page https://guide.macports.org/chunked/reference.phases.htm, I've tried a variety of options like configure.cxxflags-append=-fopenmp, all without success.

I'd also ask for consideration whether adding OpenMP support should be the default behavior for the Macports graph-tool build now that it's available.

Attachments (2)

graph_blockmodel.hh.patch (2.7 KB) - added by essandess (Steve Smith) 6 years ago.
Patch file to replace variable name "parallel" with "parallel_enabled"
graph_blockmodel_overlap.hh.patch (2.0 KB) - added by essandess (Steve Smith) 6 years ago.
Patch file to replace variable name "parallel" with "parallel_enabled"

Download all attachments as: .zip

Change History (27)

comment:1 Changed 6 years ago by mamoll (Mark Moll)

Can you change the Portfile like so:

Index: Portfile
===================================================================
--- Portfile	(revision 147061)
+++ Portfile	(working copy)
@@ -81,7 +81,7 @@
     }
     configure.cppflags-append -I${prefix}/include -I${python.include}/..
     configure.ldflags-append -L${prefix}/lib
-    configure.args-append --with-boost=${prefix} --exec-prefix=${python.prefix}
+    configure.args-append --with-boost=${prefix} --exec-prefix=${python.prefix} --enable-openmp
     # Clang uses the old libstc++ from gcc 4.2 before OS X 10.9. Boost doesn't
     # include some of the tr1 headers in libstdc++ and defines its own tr1
     # classes. This causes conflicts with sparsehash which insists on using

That seems to work for me. On my machine there is no +openmp variant for clang-3.8 (it's included by default). The clang-3.7 *does* has an +openmp variant. The version of clang that comes with the latest Xcode doesn't support openmp at all. How do I do the right thing in all cases?

comment:2 Changed 6 years ago by essandess (Steve Smith)

After applying this patch, clang-3.8 fails with the error

In file included from graph_blockmodel.cc:39:
In file included from ./graph_blockmodel_overlap.hh:24:
./graph_blockmodel.hh:2708:43: warning: missing ':' after directive name modifier - ignoring [-Wignored-pragmas]
            schedule(runtime) if (parallel)
                                          ^
./graph_blockmodel.hh:2708:43: error: expected expression

I'll go try clang-3.7...

comment:3 Changed 6 years ago by mamoll (Mark Moll)

If compilation fails, you might want to file a ticket upstream at https://git.skewed.de/count0/graph-tool/issues/.

comment:4 Changed 6 years ago by essandess (Steve Smith)

On my system clang-3.7 fails with this error:

In file included from graph_betweenness.cc:20:
In file included from /opt/local/include/boost/python.hpp:41:
/opt/local/include/boost/python/make_constructor.hpp:177:32: warning: unused typedef 'assertion' [-Wunused-local-typedef]
          >::too_many_keywords assertion;
                               ^
fatal error: error in backend: Cannot select: 0x7fe332ed22a0: f80 = zero_extend 0x7fe332ed1e00 [ORD=3] [ID=4]
  0x7fe332ed1e00: i64 = FrameIndex<14> [ID=1]
In function: .omp_outlined..24
clang: error: clang frontend command failed with exit code 70 (use -v to see invocation)

The build commands are:

sudo port install clang-3.7 +openmp
sudo port -d install py-graph-tool configure.compiler=macports-clang-3.7

comment:5 in reply to:  2 ; Changed 6 years ago by count0 (Tiago de Paula Peixoto)

Replying to s.t.smith@…:

After applying this patch, clang-3.8 fails with the error

In file included from graph_blockmodel.cc:39:
In file included from ./graph_blockmodel_overlap.hh:24:
./graph_blockmodel.hh:2708:43: warning: missing ':' after directive name modifier - ignoring [-Wignored-pragmas]
            schedule(runtime) if (parallel)
                                          ^
./graph_blockmodel.hh:2708:43: error: expected expression

This seems like a perfectly valid OpenMP annotation to me...

comment:6 in reply to:  5 ; Changed 6 years ago by essandess (Steve Smith)

Replying to tiago@…:

This seems like a perfectly valid OpenMP annotation to me...

Sounds like the upstream ticket should go to clang. I'll post one there.

Last edited 6 years ago by essandess (Steve Smith) (previous) (diff)

comment:7 in reply to:  6 Changed 6 years ago by essandess (Steve Smith)

Replying to s.t.smith@…:

Replying to tiago@…:

This seems like a perfectly valid OpenMP annotation to me...

Sounds like the upstream ticket should go to clang. I'll post one there.

Oh wait -- there's a clang-3.9. I'll attempt compiling with the latest version.

comment:8 Changed 6 years ago by essandess (Steve Smith)

Still no luck with clang-3.9. Same error as above with 3.8:

In file included from graph_blockmodel.cc:39:
In file included from ./graph_blockmodel_overlap.hh:24:
./graph_blockmodel.hh:2708:43: warning: missing ':' after directive name modifier - ignoring [-Wignored-pragmas]
            schedule(runtime) if (parallel)
                                          ^
./graph_blockmodel.hh:2708:43: error: expected expression
In file included from graph_blockmodel.cc:39:
./graph_blockmodel_overlap.hh:1337:67: warning: missing ':' after directive name modifier - ignoring [-Wignored-pragmas]
            firstprivate(m_entries) schedule(runtime) if (parallel)
                                                                  ^
./graph_blockmodel_overlap.hh:1337:67: error: expected expression
In file included from graph_blockmodel.cc:39:
In file included from ./graph_blockmodel_overlap.hh:24:
In file included from ./graph_blockmodel.hh:26:

I'll post bug report to clang.

comment:9 Changed 6 years ago by essandess (Steve Smith)

comment:10 Changed 6 years ago by essandess (Steve Smith)

The feedback from the upstream ticket mentions that omp's parallel if clause has a "parallel"-named qualifier, which causes the compilation problem. https://trac.macports.org/ticket/50983#comment:8

I confirm that replacing the variable name "parallel" with "parallel_enabled" yields a successful compile. Patch files attached.

Replying to larryv@…:

Clang supports directive name modifiers in OpenMP 4.5 if clauses. From §2.12 of the OpenMP 4.5 spec:

The syntax of the if clause is as follows:

if([directive-name-modifier :]scalar-expression)

And from §2.5:

The syntax of the parallel construct is as follows:

#pragma omp parallel [clause[[,]clause]...]new-line
structured-block

where clause is one of the following:

if([parallel :]scalar-expression)

This seems to make “parallel” a sort of reserved word that requires a trailing colon, so I doubt LLVM upstream would consider this a bug, although you’re free to submit a report. I suggest that graph-tool simply use any other variable name.

Changed 6 years ago by essandess (Steve Smith)

Attachment: graph_blockmodel.hh.patch added

Patch file to replace variable name "parallel" with "parallel_enabled"

Changed 6 years ago by essandess (Steve Smith)

Patch file to replace variable name "parallel" with "parallel_enabled"

comment:11 Changed 6 years ago by essandess (Steve Smith)

Once Tiago agrees, this gets us back to the original problem: adding OpenMP support to the py-graph-tool port.

Adding the "port" command line argument "configure.args=--enable-openmp" or "configure.args-append=--enable-openmp" doesn't append "--enable-openmp" to "./configure".

comment:12 Changed 6 years ago by mf2k (Frank Schima)

Keywords: graph-tool clang openmp libomp removed
Owner: changed from macports-tickets@… to tiago@…

comment:13 Changed 6 years ago by mamoll (Mark Moll)

Thanks for the patches. Something like this works for clang 3.[89]:

if {${configure.compiler} eq "macports-clang-3.8" ||
    ${configure.compiler} eq "macports-clang-3.9"} {
      configure.args-append --enable-openmp
}

but I can't distinguish between clang-3.7 and clang-3.7+openmp, so it seems safest to assume the clang-3.7 doesn't support OpenMP.

comment:14 in reply to:  10 Changed 6 years ago by count0 (Tiago de Paula Peixoto)

Replying to s.t.smith@…:

I confirm that replacing the variable name "parallel" with "parallel_enabled" yields a successful compile. Patch files attached.

These patches seem fine. Please submit them as well to the upstream repository at: https://git.skewed.de/count0/graph-tool

comment:15 in reply to:  13 Changed 6 years ago by essandess (Steve Smith)

Replying to mmoll@…:

Thanks for the patches. Something like this works for clang 3.[89]:

if {${configure.compiler} eq "macports-clang-3.8" ||
    ${configure.compiler} eq "macports-clang-3.9"} {
      configure.args-append --enable-openmp
}

but I can't distinguish between clang-3.7 and clang-3.7+openmp, so it seems safest to assume the clang-3.7 doesn't support OpenMP.

I'm testing with the latest upgrades and have run into the problem that clang_select shows "mp-clang-3.9", *not* "macports-clang-3.9":

$ sudo port select --list clang
Available versions for clang:
	mp-clang-3.9 (active)
	none
$ sudo port select --set clang macports-clang-3.9
Selecting 'macports-clang-3.9' for 'clang' failed: The specified version 'macports-clang-3.9' is not valid.
$ sudo port select --set clang mp-clang-3.9
Selecting 'mp-clang-3.9' for 'clang' succeeded. 'mp-clang-3.9' is now active.

The current port file doesn't detect that I've set clang-3.9 to be the default:

sudo port -d install py-graph-tool
...
checking whether to enable parallel algorithms with openmp... no

I've also confirmed that if I edit the port file to say the following, --enable-openmp is not passed to ./configure:

    if {${configure.compiler} eq "mp-clang-3.8" ||
        ${configure.compiler} eq "mp-clang-3.9"} {
        configure.args-append --enable-openmp
    }

Then this:

sudo port -d install py-graph-tool
...
checking whether to enable parallel algorithms with openmp... no

If I pass the command line argument configure.args-append=--enable-openmp, the argument is not passed:

sudo port -d install py-graph-tool configure.args-append=--enable-openmp
...
checking whether to enable parallel algorithms with openmp... no

If I change the port file back to its original, then the --enable-openmp argument is passed with the command. This is the only thing that's working so far:

sudo port -d install py-graph-tool configure.compiler=macports-clang-3.9
...
DEBUG: Assembled command: 'cd "/opt/local/var/macports/build/_opt_local_var_macports_sources_rsync.macports.org_release_tarballs_ports_python_py-graph-tool/py27-graph-tool/work/graph-tool-2.13" && ./configure --prefix=/opt/local --with-boost=/opt/local --exec-prefix=/opt/local/Library/Frameworks/Python.framework/Versions/2.7 --enable-openmp'
...
checking whether to enable parallel algorithms with openmp... yes

comment:16 Changed 6 years ago by essandess (Steve Smith)

Tiago's update from this ticket https://git.skewed.de/count0/graph-tool/issues/289 is already reflected in the mirrors, so all that's needed now is for the portfile to pass --enable-openmp to ./configure.

comment:17 Changed 6 years ago by mamoll (Mark Moll)

I don't think port select clang ... changes the default compiler that MacPorts uses. It just creates symlinks so that clang points to clang-3.9. You can change the compiler by specifying it in ${prefix}/etc/macports/macports.conf. This shouldn't be necessary:

 configure.args-append=--enable-openmp

It's already part of the Portfile.

comment:18 Changed 6 years ago by essandess (Steve Smith)

Is there a setting that will allow

sudo port upgrade outdated

or

sudo port install py-graph-tool

to automatically compile graph-tool upgrades with OpenMP support, without having to pass a configure.compiler=macports-clang-3.9 by hand?

comment:19 Changed 6 years ago by essandess (Steve Smith)

Something like the following tests whether mp-clang-3.9 is set to be the compiler, and avoids the issues of having to change macports.conf whenever clang is upgraded.

port select --show clang | perl -ne "chomp; s/The currently selected version for 'clang' is '(.*)'./\$1/; print $_;"

comment:20 Changed 6 years ago by mamoll (Mark Moll)

We can add clang38/clang39 variants:

variant clang38 description "Use clang-3.8 and enable OpenMP" conflicts clang39 {
      configure.compiler  macports-clang-3.8
}

variant clang38 description "Use clang-3.9 and enable OpenMP" conflicts clang38 {
      configure.compiler  macports-clang-3.9
}

The next question is then do you think +clang38 should be a default variant?

comment:21 in reply to:  20 ; Changed 6 years ago by essandess (Steve Smith)

Replying to mmoll@…:

We can add clang38/clang39 variants:

variant clang38 description "Use clang-3.8 and enable OpenMP" conflicts clang39 {
      configure.compiler  macports-clang-3.8
}

variant clang38 description "Use clang-3.9 and enable OpenMP" conflicts clang38 {
      configure.compiler  macports-clang-3.9
}

The next question is then do you think +clang38 should be a default variant?

Yes, variants are a great idea.

My personal preference would be default OpenMP support because multithreading makes my code scream on large graphs, and all my boxes have multiple cores anyway. I suspect this is common among graph-tool users, although I'd defer to Tiago or others who have experience with omp build robustness.

comment:22 Changed 6 years ago by essandess (Steve Smith)

PS typo in the clang39 variant if you go that route.

comment:23 in reply to:  21 Changed 6 years ago by count0 (Tiago de Paula Peixoto)

My personal preference would be default OpenMP support because multithreading makes my code scream on large graphs, and all my boxes have multiple cores anyway. I suspect this is common among graph-tool users, although I'd defer to Tiago or others who have experience with omp build robustness.

I always enable openmp with GCC, and it is the default on all the packages I maintain (Debian/Ubuntu and Arch). Assuming the clang implementation is stable (I have no idea), it should be the default.

comment:24 Changed 6 years ago by essandess (Steve Smith)

Rather than a "clang39" variant, I'd suggest an "openmp" variant that would specify whatever the latest clang version is and pass configure.args-append=--enable-openmp. And make this variant the default as Tiago says.

comment:25 Changed 6 years ago by mamoll (Mark Moll)

Resolution: fixed
Status: newclosed

Fixed in r147229.

Note: See TracTickets for help on using tickets.