Patchwork Cloog ISL - and linking of libisl

login
register
mail settings
Submitter Tobias Burnus
Date Feb. 15, 2011, 7:07 p.m.
Message ID <4D5ACEFF.8090405@net-b.de>
Download mbox | patch
Permalink /patch/83284/
State New
Headers show

Comments

Tobias Burnus - Feb. 15, 2011, 7:07 p.m.
Dear Sebastian and Tobias, hello all,

I think GCC should automatically link libisl ("-lisl") if one configures 
GCC to use cloog-isl. That's the first patch at 
http://gcc.gnu.org/ml/gcc-patches/2010-11/msg01272.html , which remains 
unreviewed. (The second part about "-lpwl" looks a bit odd.)

The issue came also up today at #gfortran - and the proposed solutions 
was the same.

I think automatically linking libisl makes sense and is also in line 
with PPL, which is linked via the toplevel configure.ac (cf. second - 
bogus - part of the linked patch or simply the file itself).

The patch I am talking about is:




Tobias
Sebastian Pop - Feb. 15, 2011, 7:38 p.m.
Hi,

On Tue, Feb 15, 2011 at 13:07, Tobias Burnus <burnus@net-b.de> wrote:
> Dear Sebastian and Tobias, hello all,
>
> I think GCC should automatically link libisl ("-lisl") if one configures GCC
> to use cloog-isl. That's the first patch at
> http://gcc.gnu.org/ml/gcc-patches/2010-11/msg01272.html , which remains
> unreviewed. (The second part about "-lpwl" looks a bit odd.)
>
> The issue came also up today at #gfortran - and the proposed solutions was
> the same.
>
> I think automatically linking libisl makes sense and is also in line with
> PPL, which is linked via the toplevel configure.ac (cf. second - bogus -
> part of the linked patch or simply the file itself).
>
> The patch I am talking about is:
>
> Index: config/cloog.m4
> ===================================================================
> --- config/cloog.m4     (revision 166641)
> +++ config/cloog.m4     (working copy)
> @@ -143,7 +143,7 @@
>       ;;
>     "ISL")
>       clooginc="${clooginc} ${_cloogorginc}"
> -      clooglibs="${clooglibs} -lcloog-isl"
> +      clooglibs="${clooglibs} -lcloog-isl -lisl"

I think that this change makes sense but I cannot approve it.
Paolo, could you please review this as well?

Thanks,
Sebastian

>       cloog_org=yes
>       ;;
>     "PPL")
>
>
>
> Tobias
>
Ralf Wildenhues - Feb. 15, 2011, 8:34 p.m.
* Sebastian Pop wrote on Tue, Feb 15, 2011 at 08:38:28PM CET:
> On Tue, Feb 15, 2011 at 13:07, Tobias Burnus wrote:
> > --- config/cloog.m4     (revision 166641)
> > +++ config/cloog.m4     (working copy)
> > @@ -143,7 +143,7 @@
> >       ;;
> >     "ISL")
> >       clooginc="${clooginc} ${_cloogorginc}"
> > -      clooglibs="${clooglibs} -lcloog-isl"
> > +      clooglibs="${clooglibs} -lcloog-isl -lisl"
> 
> I think that this change makes sense but I cannot approve it.
> Paolo, could you please review this as well?

I'm not Paolo.  But the patch is OK if libisl is always needed for this
configuration.

Does GCC call functions from libisl directly?  If yes, then adding -lisl
is unconditionally a good thing.  If not, then this should primarily
help for static linking.  I assume that GCC cares less about the odd
extra library linked against than a broken link (witness the ppllibs in
configure.ac).

> >       cloog_org=yes
> >       ;;
> >     "PPL")

Thanks,
Ralf
Sebastian Pop - Feb. 15, 2011, 9:19 p.m.
> I'm not Paolo.  But the patch is OK if libisl is always needed for this
> configuration.

Thanks Ralf, I will regstrap the patch and commit it later today.

> Does GCC call functions from libisl directly?  If yes, then adding -lisl
> is unconditionally a good thing.  If not, then this should primarily
> help for static linking.

In 4.6 there are no ISL functions called directly from GCC.
In 4.7 we plan to use some of ISL's functionality (array data flow analysis)
to check (or replace) the same analysis of GCC.

> I assume that GCC cares less about the odd
> extra library linked against than a broken link (witness the ppllibs in
> configure.ac).

The watchdog library is required only when the PPL is configured with
--enable-watchdog.  This is not a requirement of GCC, but could help
to fix the linking problem reported.  Is the second patch of
http://gcc.gnu.org/ml/gcc-patches/2010-11/msg01272.html
ok for trunk as well?

Thanks,
Sebastian
Ralf Wildenhues - Feb. 15, 2011, 9:39 p.m.
* Sebastian Pop wrote on Tue, Feb 15, 2011 at 10:19:32PM CET:
> The watchdog library is required only when the PPL is configured with
> --enable-watchdog.  This is not a requirement of GCC, but could help
> to fix the linking problem reported.  Is the second patch of
> http://gcc.gnu.org/ml/gcc-patches/2010-11/msg01272.html
> ok for trunk as well?

Where is the bug report for this?  Is it possible to find out whether
PPL was configured with --enable-watchdog?  Does PPL install a
pkg-config .pc file or a libtool .la file from which one could gather
the required information automatically?

Is libpwl even built and installed from PPL if --enable-watchdog was not
passed to its configure script (or even --disable-watchdog was passed)?
Are the two libraries distributed in the same package on all common
distributions (or, if in two packages, does the one with PPL routinely
depend on the pwl one)?

Thanks,
Ralf
John Tytgat - Feb. 15, 2011, 10:16 p.m.
In message <20110215213906.GI24361@gmx.de>
          Ralf Wildenhues <Ralf.Wildenhues@gmx.de> wrote:

> * Sebastian Pop wrote on Tue, Feb 15, 2011 at 10:19:32PM CET:
> > The watchdog library is required only when the PPL is configured with
> > --enable-watchdog.  This is not a requirement of GCC, but could help
> > to fix the linking problem reported.  Is the second patch of
> > http://gcc.gnu.org/ml/gcc-patches/2010-11/msg01272.html
> > ok for trunk as well?
> 
> Where is the bug report for this?  Is it possible to find out whether
> PPL was configured with --enable-watchdog?  Does PPL install a
> pkg-config .pc file or a libtool .la file from which one could gather
> the required information automatically?
> 
> Is libpwl even built and installed from PPL if --enable-watchdog was not
> passed to its configure script (or even --disable-watchdog was passed)?
> Are the two libraries distributed in the same package on all common
> distributions (or, if in two packages, does the one with PPL routinely
> depend on the pwl one)?

No bug report but see http://gcc.gnu.org/ml/gcc-patches/2011-01/msg01946.html
especially http://gcc.gnu.org/ml/gcc-patches/2011-01/msg02177.html

Looks like PPL's pkg-config needs fixing.

John.
Alexandre Oliva - Feb. 16, 2011, 1:07 a.m.
On Feb 15, 2011, Tobias Burnus <burnus@net-b.de> wrote:

> -      clooglibs="${clooglibs} -lcloog-isl"
> +      clooglibs="${clooglibs} -lcloog-isl -lisl"

This is ok, thanks.
Ralf Wildenhues - Feb. 16, 2011, 5:57 a.m.
* John Tytgat wrote on Tue, Feb 15, 2011 at 11:16:16PM CET:
> In message <20110215213906.GI24361@gmx.de>
>           Ralf Wildenhues wrote:
> > * Sebastian Pop wrote on Tue, Feb 15, 2011 at 10:19:32PM CET:
> > > The watchdog library is required only when the PPL is configured with
> > > --enable-watchdog.  This is not a requirement of GCC, but could help
> > > to fix the linking problem reported.  Is the second patch of
> > > http://gcc.gnu.org/ml/gcc-patches/2010-11/msg01272.html
> > > ok for trunk as well?
> > 
> > Where is the bug report for this?  Is it possible to find out whether
> > PPL was configured with --enable-watchdog?  Does PPL install a
> > pkg-config .pc file or a libtool .la file from which one could gather
> > the required information automatically?
> > 
> > Is libpwl even built and installed from PPL if --enable-watchdog was not
> > passed to its configure script (or even --disable-watchdog was passed)?
> > Are the two libraries distributed in the same package on all common
> > distributions (or, if in two packages, does the one with PPL routinely
> > depend on the pwl one)?
> 
> No bug report but see http://gcc.gnu.org/ml/gcc-patches/2011-01/msg01946.html
> especially http://gcc.gnu.org/ml/gcc-patches/2011-01/msg02177.html

In that case the patch is not OK.

> Looks like PPL's pkg-config needs fixing.

I guess.

For GCC, a more acceptable alternative than the proposed patch would be
to actually do a link test for libpwl (using AC_CHECK_LIB for example)
and only adding it based on the result.  Such a change wouldn't require
a newer PPL with fixed pkg-config data.

Thanks,
Ralf
Jack Howarth - April 6, 2011, 10:20 p.m.
On Tue, Feb 15, 2011 at 08:07:43PM +0100, Tobias Burnus wrote:
> Dear Sebastian and Tobias, hello all,
>
> I think GCC should automatically link libisl ("-lisl") if one configures  
> GCC to use cloog-isl. That's the first patch at  
> http://gcc.gnu.org/ml/gcc-patches/2010-11/msg01272.html , which remains  
> unreviewed. (The second part about "-lpwl" looks a bit odd.)
>
> The issue came also up today at #gfortran - and the proposed solutions  
> was the same.
>
> I think automatically linking libisl makes sense and is also in line  
> with PPL, which is linked via the toplevel configure.ac (cf. second -  
> bogus - part of the linked patch or simply the file itself).
>
> The patch I am talking about is:
>
> Index: config/cloog.m4
> ===================================================================
> --- config/cloog.m4	(revision 166641)
> +++ config/cloog.m4	(working copy)
> @@ -143,7 +143,7 @@
>        ;;
>      "ISL")
>        clooginc="${clooginc} ${_cloogorginc}"
> -      clooglibs="${clooglibs} -lcloog-isl"
> +      clooglibs="${clooglibs} -lcloog-isl -lisl"
>        cloog_org=yes
>        ;;
>      "PPL")
>
>
>
> Tobias

Tobias,
   Are we certain that it is essential to explicitly link in -lisl?
This change has caused an unnecessary rebuild of FSF gcc when upgrading
cloog.org from 0.16.1 to 0.16.2 due to the soversion bump on libisl.
Do we know of any direct calls from graphite into libisl? If all accesses
are done through the API in libcloog-isl, which hasn't been version bumped,
the extra linkage on -lisl would appear to be unnecessary.
          Jack
Ralf Wildenhues - April 7, 2011, 6:10 a.m.
* Jack Howarth wrote on Thu, Apr 07, 2011 at 12:20:00AM CEST:
> On Tue, Feb 15, 2011 at 08:07:43PM +0100, Tobias Burnus wrote:
> > --- config/cloog.m4	(revision 166641)
> > +++ config/cloog.m4	(working copy)
> > @@ -143,7 +143,7 @@
> >        ;;
> >      "ISL")
> >        clooginc="${clooginc} ${_cloogorginc}"
> > -      clooglibs="${clooglibs} -lcloog-isl"
> > +      clooglibs="${clooglibs} -lcloog-isl -lisl"
> >        cloog_org=yes
> >        ;;
> >      "PPL")

>    Are we certain that it is essential to explicitly link in -lisl?
> This change has caused an unnecessary rebuild of FSF gcc when upgrading
> cloog.org from 0.16.1 to 0.16.2 due to the soversion bump on libisl.
> Do we know of any direct calls from graphite into libisl? If all accesses
> are done through the API in libcloog-isl, which hasn't been version bumped,
> the extra linkage on -lisl would appear to be unnecessary.

What about static only links?

Cheers,
Ralf

Patch

Index: config/cloog.m4
===================================================================
--- config/cloog.m4	(revision 166641)
+++ config/cloog.m4	(working copy)
@@ -143,7 +143,7 @@ 
        ;;
      "ISL")
        clooginc="${clooginc} ${_cloogorginc}"
-      clooglibs="${clooglibs} -lcloog-isl"
+      clooglibs="${clooglibs} -lcloog-isl -lisl"
        cloog_org=yes
        ;;
      "PPL")