diff mbox

Support official CLooG.org versions.

Message ID 4CDD67EE.6070807@fim.uni-passau.de
State New
Headers show

Commit Message

Tobias Grosser Nov. 12, 2010, 4:14 p.m. UTC 
On 11/12/2010 04:10 AM, Paolo Bonzini wrote:
> On 11/12/2010 10:08 AM, Sven Verdoolaege wrote:
>> On Fri, Nov 12, 2010 at 09:59:45AM +0100, Paolo Bonzini wrote:
>>> On 11/12/2010 09:43 AM, Sven Verdoolaege wrote:
>>>> On Fri, Nov 12, 2010 at 09:26:00AM +0100, Paolo Bonzini wrote:
>>>>> Also, if I understood correctly ISL and PPL are different ways to
>>>>> "do the same thing", and they should cause no differences in code
>>>>> generation. I assumed this because the patch didn't require any
>>>>> testsuite adjustment. Is this the case?
>>>>
>>>> Semantically, the results should be the same, but there may
>>>> be syntactic differences. Perhaps no such syntactic differences
>>>> occur for the gcc testsuite.
>>>
>>> What does "semantic" and "syntactic" mean? I suppose you mean that
>>> the produced code should be correct in any case (of course) but the
>>> GCC assembly language output may change?
>>
>> Exactly.
>
> That's bad. Sebastian, please revert the patch. It would also be
> appreciated to compile SPEC with both backends, and see how many
> different decisions are taken.

Hi,

I just tested this again and Jack Howarth's analysis was correct. CLooG 
PPL is not detected, if PPL is not in the default path. This slipped 
through our tests. As long as PPL is installed in the system library 
path everything is all right.

This is definitely a bug in the patch, as the patch was not intended to 
change the default behavior at all. I propose to fix this bug.
A patch for this is attached (the one Jack proposed).

Regarding the syntactic difference:

CLooG isl will in some cases generate different code. This is why we 
currently have support for both CLooG versions. Our first tests did not 
show any new SPEC suite failures, but we still want to run more tests 
with different optimization flags and on even more platforms. We push 
that patch upstream, such that people like Jack can test CLooG with 
CLooG-isl on their platform. (Even if it was not intended they hit such 
a simple configure bug)

I believe with the attached patch, the know CLooG configure bugs are 
fixed and configure should act as it was planned.

The behavior is:

configure will detect CLooG PPL if available in the default search paths 
or pointed to using the --with-cloog flag.
Only in the case no CLooG PPL is available but a version of CLooG ISL is 
installed or pointed explicitly to using the --with-cloog flag, gcc will 
compile against cloog-isl.

cloog-isl is currently unreleased (only available using git).

Is this behavior OK for configure? The idea is to keep it like this for 
a couple of weeks, such that more people can try CLooG isl and help us 
finding the remaining bugs.

The final goal for gcc 4.6 is to remove CLooG ppl support.

Is this plan as explained and finally implemented (with the attached 
fix) OK, or do we need to change anything else?

Cheers
Tobi

Comments

Paolo Bonzini Nov. 12, 2010, 5:06 p.m. UTC | #1 
On 11/12/2010 05:14 PM, Tobias Grosser wrote:
> CLooG isl will in some cases generate different code. This is why we
> currently have support for both CLooG versions. Our first tests did not
> show any new SPEC suite failures, but we still want to run more tests
> with different optimization flags and on even more platforms. We push
> that patch upstream, such that people like Jack can test CLooG with
> CLooG-isl on their platform. (Even if it was not intended they hit such
> a simple configure bug)

Yes, the bug that Jack found is not a major problem.

The main problem is the different generated code; in particular, it's 
not about failures, _any_ assembly code difference resulting from the 
same configure options is problematic.  Can you make a patch to ensure 
that only one backend is sought for, depending on the user's choice 
between --enable-cloog-backend=ppl and --enable-cloog-backend=isl (you 
choose the default)?

To some extent, this is unavoidable when we are delegating optimization 
choices to an external library, but we should minimize the differences. 
  It is probably necessary to have the version of cloog (and its 
dependencies whenever applicable) printed as part of the output of "gcc 
--version".

Still, the patch is okay. :)

Paolo
Jack Howarth Nov. 12, 2010, 6:32 p.m. UTC | #2 
On Fri, Nov 12, 2010 at 11:14:38AM -0500, Tobias Grosser wrote:
> On 11/12/2010 04:10 AM, Paolo Bonzini wrote:
>> On 11/12/2010 10:08 AM, Sven Verdoolaege wrote:
>>> On Fri, Nov 12, 2010 at 09:59:45AM +0100, Paolo Bonzini wrote:
>>>> On 11/12/2010 09:43 AM, Sven Verdoolaege wrote:
>>>>> On Fri, Nov 12, 2010 at 09:26:00AM +0100, Paolo Bonzini wrote:
>>>>>> Also, if I understood correctly ISL and PPL are different ways to
>>>>>> "do the same thing", and they should cause no differences in code
>>>>>> generation. I assumed this because the patch didn't require any
>>>>>> testsuite adjustment. Is this the case?
>>>>>
>>>>> Semantically, the results should be the same, but there may
>>>>> be syntactic differences. Perhaps no such syntactic differences
>>>>> occur for the gcc testsuite.
>>>>
>>>> What does "semantic" and "syntactic" mean? I suppose you mean that
>>>> the produced code should be correct in any case (of course) but the
>>>> GCC assembly language output may change?
>>>
>>> Exactly.
>>
>> That's bad. Sebastian, please revert the patch. It would also be
>> appreciated to compile SPEC with both backends, and see how many
>> different decisions are taken.
>
> Hi,
>
> I just tested this again and Jack Howarth's analysis was correct. CLooG  
> PPL is not detected, if PPL is not in the default path. This slipped  
> through our tests. As long as PPL is installed in the system library  
> path everything is all right.

Tobias,
   Are you sure your analysis is correct? It seems to me that the problem
is actually due to the code section in config/cloog.m4 below...

# CLOOG_FIND_FLAGS ()
# ------------------
# Detect the used CLooG-backend and set clooginc/clooglibs/cloog_org.
# Preference: CLooG-PPL (Legacy) > CLooG-ISL > CLooG-PPL
AC_DEFUN([CLOOG_FIND_FLAGS],
[
  AC_REQUIRE([CLOOG_INIT_FLAGS])

  _cloog_saved_CFLAGS=$CFLAGS
  _cloog_saved_CPPFLAGS=$CPPFLAGS
  _cloog_saved_LDFLAGS=$LDFLAGS
  _cloog_saved_LIBS=$LIBS

  _clooglegacyinc="-DCLOOG_PPL_BACKEND"
  _cloogorginc="-DCLOOG_INT_GMP -DCLOOG_ORG"
 
  dnl clooglibs & clooginc may have been initialized by CLOOG_INIT_FLAGS.
  CFLAGS="${CFLAGS} ${clooginc} ${pplinc} ${gmpinc}"
  CPPFLAGS="${CPPFLAGS} ${_clooglegacyinc} ${_cloogorginc}"
  LDFLAGS="${LDFLAGS} ${clooglibs}"

  AC_CACHE_CHECK([for installed CLooG],
                 [gcc_cv_cloog_type],
    [LIBS="-lcloog ${_cloog_saved_LIBS}"
     AC_LINK_IFELSE([_CLOOG_PPL_LEGACY_PROG],
      [gcc_cv_cloog_type="PPL Legacy"],
      [LIBS="-lcloog-isl -lisl ${_cloog_saved_LIBS}"
       AC_LINK_IFELSE([_CLOOG_ORG_PROG],
        [gcc_cv_cloog_type=ISL],
        [LIBS="-lcloog-ppl ${_cloog_saved_LIBS}"
         AC_LINK_IFELSE([_CLOOG_ORG_PROG],
          [gcc_cv_cloog_type=PPL],
          [gcc_cv_cloog_type=no])])])])

  case $gcc_cv_cloog_type in
    "PPL Legacy")
      clooginc="${clooginc} ${_clooglegacyinc}"
      clooglibs="${clooglibs} -lcloog"
      cloog_org=no
      ;;
    "ISL")
      clooginc="${clooginc} ${_cloogorginc}"
      clooglibs="${clooglibs} -lcloog-isl"
      cloog_org=yes
      ;;
    "PPL")
      clooginc="${clooginc} ${_cloogorginc}"
      clooglibs="${clooglibs} -lcloog-ppl"
      cloog_org=yes
      ;;
    *)
      clooglibs=
      clooginc=
      cloog_org=
      ;;
  esac

  LIBS=$_cloog_saved_LIBS
  CFLAGS=$_cloog_saved_CFLAGS
  CPPFLAGS=$_cloog_saved_CPPFLAGS
  LDFLAGS=$_cloog_saved_LDFLAGS
]
)

In particular, the lines...

 dnl clooglibs & clooginc may have been initialized by CLOOG_INIT_FLAGS.
  CFLAGS="${CFLAGS} ${clooginc} ${pplinc} ${gmpinc}"
  CPPFLAGS="${CPPFLAGS} ${_clooglegacyinc} ${_cloogorginc}"
  LDFLAGS="${LDFLAGS} ${clooglibs}"

where ${pplinc} is blindly passed to all the tests on CFLAGS but
${ppllibs} is never passed to LDFLAGS for the legacy cloog test.
Since only the legacy cloog test directly makes a call into ppl...

# _CLOOG_PPL_LEGACY_PROG ()
# -------------------------
# Helper for detecting CLooG-Legacy (CLooG-PPL).
m4_define([_CLOOG_PPL_LEGACY_PROG], [AC_LANG_PROGRAM(
  [#include <cloog/cloog.h>],
  [ppl_version_major ()])])

...it would seem that ${pplinc} and ${ppllibs} only need to be
provided to CFLAGS and LDFLAGS respectively in that case.
          Jack

> This is definitely a bug in the patch, as the patch was not intended to  
> change the default behavior at all. I propose to fix this bug.
> A patch for this is attached (the one Jack proposed).
>
> Regarding the syntactic difference:
>
> CLooG isl will in some cases generate different code. This is why we  
> currently have support for both CLooG versions. Our first tests did not  
> show any new SPEC suite failures, but we still want to run more tests  
> with different optimization flags and on even more platforms. We push  
> that patch upstream, such that people like Jack can test CLooG with  
> CLooG-isl on their platform. (Even if it was not intended they hit such  
> a simple configure bug)
>
> I believe with the attached patch, the know CLooG configure bugs are  
> fixed and configure should act as it was planned.
>
> The behavior is:
>
> configure will detect CLooG PPL if available in the default search paths  
> or pointed to using the --with-cloog flag.
> Only in the case no CLooG PPL is available but a version of CLooG ISL is  
> installed or pointed explicitly to using the --with-cloog flag, gcc will  
> compile against cloog-isl.
>
> cloog-isl is currently unreleased (only available using git).
>
> Is this behavior OK for configure? The idea is to keep it like this for  
> a couple of weeks, such that more people can try CLooG isl and help us  
> finding the remaining bugs.
>
> The final goal for gcc 4.6 is to remove CLooG ppl support.
>
> Is this plan as explained and finally implemented (with the attached  
> fix) OK, or do we need to change anything else?
>
> Cheers
> Tobi

> >From a8d97d865ab5e0af7e1a760ee6769dfb877231a1 Mon Sep 17 00:00:00 2001
> From: Tobias Grosser <grosser@fim.uni-passau.de>
> Date: Thu, 11 Nov 2010 22:54:55 -0500
> Subject: [PATCH 2/2] Pass PPL libraries to CLooG version check
> 
> 	* config/cloog.m4: Pass ppl libraries to the CLooG version check.
> 	* configure: Regenerate.
> ---
>  config/cloog.m4 |    4 ++--
>  configure       |    4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/config/cloog.m4 b/config/cloog.m4
> index d24b5ac..87ff50a 100644
> --- a/config/cloog.m4
> +++ b/config/cloog.m4
> @@ -122,7 +122,7 @@ AC_DEFUN([CLOOG_FIND_FLAGS],
>    dnl clooglibs & clooginc may have been initialized by CLOOG_INIT_FLAGS.
>    CFLAGS="${CFLAGS} ${clooginc} ${pplinc} ${gmpinc}"
>    CPPFLAGS="${CPPFLAGS} ${_clooglegacyinc} ${_cloogorginc}"
> -  LDFLAGS="${LDFLAGS} ${clooglibs}"
> +  LDFLAGS="${LDFLAGS} ${clooglibs} ${ppllibs}"
>  
>    AC_CACHE_CHECK([for installed CLooG],
>                   [gcc_cv_cloog_type],
> @@ -206,7 +206,7 @@ AC_DEFUN([CLOOG_CHECK_VERSION],
>      _cloog_saved_LDFLAGS=$LDFLAGS
>  
>      CFLAGS="${_cloog_saved_CFLAGS} ${clooginc} ${pplinc} ${gmpinc}"
> -    LDFLAGS="${_cloog_saved_LDFLAGS} ${clooglibs}"
> +    LDFLAGS="${_cloog_saved_LDFLAGS} ${clooglibs} ${ppllibs}"
>  
>      if test "${cloog_org}" = yes ; then
>        AC_CACHE_CHECK([for verison $1.$2.$3 of CLooG],
> diff --git a/configure b/configure
> index 3f8c26a..a85874e 100755
> --- a/configure
> +++ b/configure
> @@ -5730,7 +5730,7 @@ if test "x$with_cloog" != "xno"; then
>  
>      CFLAGS="${CFLAGS} ${clooginc} ${pplinc} ${gmpinc}"
>    CPPFLAGS="${CPPFLAGS} ${_clooglegacyinc} ${_cloogorginc}"
> -  LDFLAGS="${LDFLAGS} ${clooglibs}"
> +  LDFLAGS="${LDFLAGS} ${clooglibs} ${ppllibs}"
>  
>    { $as_echo "$as_me:${as_lineno-$LINENO}: checking for installed CLooG" >&5
>  $as_echo_n "checking for installed CLooG... " >&6; }
> @@ -5835,7 +5835,7 @@ $as_echo "$gcc_cv_cloog_type" >&6; }
>      _cloog_saved_LDFLAGS=$LDFLAGS
>  
>      CFLAGS="${_cloog_saved_CFLAGS} ${clooginc} ${pplinc} ${gmpinc}"
> -    LDFLAGS="${_cloog_saved_LDFLAGS} ${clooglibs}"
> +    LDFLAGS="${_cloog_saved_LDFLAGS} ${clooglibs} ${ppllibs}"
>  
>      if test "${cloog_org}" = yes ; then
>        { $as_echo "$as_me:${as_lineno-$LINENO}: checking for verison 0.14.0 of CLooG" >&5
> -- 
> 1.7.1
>
diff mbox

Patch

From a8d97d865ab5e0af7e1a760ee6769dfb877231a1 Mon Sep 17 00:00:00 2001
From: Tobias Grosser <grosser@fim.uni-passau.de>
Date: Thu, 11 Nov 2010 22:54:55 -0500
Subject: [PATCH 2/2] Pass PPL libraries to CLooG version check

	* config/cloog.m4: Pass ppl libraries to the CLooG version check.
	* configure: Regenerate.
---
 config/cloog.m4 |    4 ++--
 configure       |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/config/cloog.m4 b/config/cloog.m4
index d24b5ac..87ff50a 100644
--- a/config/cloog.m4
+++ b/config/cloog.m4
@@ -122,7 +122,7 @@  AC_DEFUN([CLOOG_FIND_FLAGS],
   dnl clooglibs & clooginc may have been initialized by CLOOG_INIT_FLAGS.
   CFLAGS="${CFLAGS} ${clooginc} ${pplinc} ${gmpinc}"
   CPPFLAGS="${CPPFLAGS} ${_clooglegacyinc} ${_cloogorginc}"
-  LDFLAGS="${LDFLAGS} ${clooglibs}"
+  LDFLAGS="${LDFLAGS} ${clooglibs} ${ppllibs}"
 
   AC_CACHE_CHECK([for installed CLooG],
                  [gcc_cv_cloog_type],
@@ -206,7 +206,7 @@  AC_DEFUN([CLOOG_CHECK_VERSION],
     _cloog_saved_LDFLAGS=$LDFLAGS
 
     CFLAGS="${_cloog_saved_CFLAGS} ${clooginc} ${pplinc} ${gmpinc}"
-    LDFLAGS="${_cloog_saved_LDFLAGS} ${clooglibs}"
+    LDFLAGS="${_cloog_saved_LDFLAGS} ${clooglibs} ${ppllibs}"
 
     if test "${cloog_org}" = yes ; then
       AC_CACHE_CHECK([for verison $1.$2.$3 of CLooG],
diff --git a/configure b/configure
index 3f8c26a..a85874e 100755
--- a/configure
+++ b/configure
@@ -5730,7 +5730,7 @@  if test "x$with_cloog" != "xno"; then
 
     CFLAGS="${CFLAGS} ${clooginc} ${pplinc} ${gmpinc}"
   CPPFLAGS="${CPPFLAGS} ${_clooglegacyinc} ${_cloogorginc}"
-  LDFLAGS="${LDFLAGS} ${clooglibs}"
+  LDFLAGS="${LDFLAGS} ${clooglibs} ${ppllibs}"
 
   { $as_echo "$as_me:${as_lineno-$LINENO}: checking for installed CLooG" >&5
 $as_echo_n "checking for installed CLooG... " >&6; }
@@ -5835,7 +5835,7 @@  $as_echo "$gcc_cv_cloog_type" >&6; }
     _cloog_saved_LDFLAGS=$LDFLAGS
 
     CFLAGS="${_cloog_saved_CFLAGS} ${clooginc} ${pplinc} ${gmpinc}"
-    LDFLAGS="${_cloog_saved_LDFLAGS} ${clooglibs}"
+    LDFLAGS="${_cloog_saved_LDFLAGS} ${clooglibs} ${ppllibs}"
 
     if test "${cloog_org}" = yes ; then
       { $as_echo "$as_me:${as_lineno-$LINENO}: checking for verison 0.14.0 of CLooG" >&5
-- 
1.7.1