[PATCH] Fix PR47566

[PATCH] Fix PR47566

[Richard Guenther]

When inserting calls in PRE we do fold them, which, when presented
by an unfolded call can result in a load of stmts to appear
(like in the testcase the equivalent of sqrt(imag(x)**2+real(x)**2)).
This is not a problem in principle, but gimplifying the stmts causes
us to insert non-register temporaries when gimplifying save-exprs.
I'm not sure why save-expr gimplifying is not using registers, but
the following patch avoids using save-exprs around SSA_NAMEs which
is clearly unnecessary.

Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.

Richard.

2011-02-02  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/47566
	* builtins.c (builtin_save_expr): No SAVE_EXPR for SSA_NAMEs.

	* gcc.dg/lto/20110201-1_0.c: New testcase.

Index: gcc/builtins.c
===================================================================
*** gcc/builtins.c	(revision 169486)
--- gcc/builtins.c	(working copy)
*************** target_char_cast (tree cst, char *p)
*** 652,660 ****
  static tree
  builtin_save_expr (tree exp)
  {
!   if (TREE_ADDRESSABLE (exp) == 0
!       && (TREE_CODE (exp) == PARM_DECL
! 	  || (TREE_CODE (exp) == VAR_DECL && !TREE_STATIC (exp))))
      return exp;
  
    return save_expr (exp);
--- 652,661 ----
  static tree
  builtin_save_expr (tree exp)
  {
!   if (TREE_CODE (exp) == SSA_NAME
!       || (TREE_ADDRESSABLE (exp) == 0
! 	  && (TREE_CODE (exp) == PARM_DECL
! 	      || (TREE_CODE (exp) == VAR_DECL && !TREE_STATIC (exp)))))
      return exp;
  
    return save_expr (exp);
Index: gcc/testsuite/gcc.dg/lto/20110201-1_0.c
===================================================================
*** gcc/testsuite/gcc.dg/lto/20110201-1_0.c	(revision 0)
--- gcc/testsuite/gcc.dg/lto/20110201-1_0.c	(revision 0)
***************
*** 0 ****
--- 1,19 ----
+ /* { dg-lto-do run } */
+ /* { dg-lto-options { { -O0 -flto } } } */
+ /* { dg-extra-ld-options "-O2 -ffast-math -fuse-linker-plugin" } */
+ /* { dg-require-linker-plugin "" } */
+ 
+ /* We require a linker plugin because otherwise we'd need to link
+    against libm which we are not sure here has cabs on all targets.
+    This is because collect2 invokes ld on the -O0 object code
+    which does not have folded cabs.  */
+ 
+ double cabs(_Complex double);
+ double __attribute__((used))
+ foo (_Complex double x, int b)
+ {
+   if (b)
+     x = 0;
+   return cabs(x);
+ }
+ int main() { return 0; }

Re: [PATCH] Fix PR47566

[Ulrich Weigand]

Richard Guenther wrote:

> + /* { dg-lto-do run } */
> + /* { dg-lto-options { { -O0 -flto } } } */
> + /* { dg-extra-ld-options "-O2 -ffast-math -fuse-linker-plugin" } */
> + /* { dg-require-linker-plugin "" } */
> + 
> + /* We require a linker plugin because otherwise we'd need to link
> +    against libm which we are not sure here has cabs on all targets.
> +    This is because collect2 invokes ld on the -O0 object code
> +    which does not have folded cabs.  */
> + 
> + double cabs(_Complex double);
> + double __attribute__((used))
> + foo (_Complex double x, int b)
> + {
> +   if (b)
> +     x = 0;
> +   return cabs(x);
> + }
> + int main() { return 0; }

Now that we have the linker plugin, this fails on spu-elf with:

/tmp/cce6KuRb.ltrans0.ltrans.o: In function `foo':
cce6KuRb.ltrans0.o:(.text+0x28): undefined reference to `sqrt'

because nothing links against libm.  I'm a bit confused by the comment
above: even with the linker plugin, where should libm be pulled in?
Don't we need to use -lm on the linker line anyway?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] Fix PR47566

[Richard Guenther]

On Fri, 22 Jul 2011, Ulrich Weigand wrote:

> Richard Guenther wrote:
> 
> > + /* { dg-lto-do run } */
> > + /* { dg-lto-options { { -O0 -flto } } } */
> > + /* { dg-extra-ld-options "-O2 -ffast-math -fuse-linker-plugin" } */
> > + /* { dg-require-linker-plugin "" } */
> > + 
> > + /* We require a linker plugin because otherwise we'd need to link
> > +    against libm which we are not sure here has cabs on all targets.
> > +    This is because collect2 invokes ld on the -O0 object code
> > +    which does not have folded cabs.  */
> > + 
> > + double cabs(_Complex double);
> > + double __attribute__((used))
> > + foo (_Complex double x, int b)
> > + {
> > +   if (b)
> > +     x = 0;
> > +   return cabs(x);
> > + }
> > + int main() { return 0; }
> 
> Now that we have the linker plugin, this fails on spu-elf with:
> 
> /tmp/cce6KuRb.ltrans0.ltrans.o: In function `foo':
> cce6KuRb.ltrans0.o:(.text+0x28): undefined reference to `sqrt'
> 
> because nothing links against libm.  I'm a bit confused by the comment
> above: even with the linker plugin, where should libm be pulled in?
> Don't we need to use -lm on the linker line anyway?

Well, the testcase was for an ICE which required that cabs is folded
to sqrt(real**2 + imag**2), it requires "late" folding, thus
-O0 during the compile step (it's a trick to use LTO here, to delay
folding until after IPA opts).  Not linking with -lm and relying
on a HW sqrt instruction makes sure we do that late folding
which we want to happen.

So, if you can in any way retain all of the above features with
fixing non-HW sqrt targets fine - otherwise I suggest you skip spu-elf.

Richard.

Re: [PATCH] Fix PR47566

[Ulrich Weigand]

Richard Guenther wrote:
> On Fri, 22 Jul 2011, Ulrich Weigand wrote:
> > Now that we have the linker plugin, this fails on spu-elf with:
> > 
> > /tmp/cce6KuRb.ltrans0.ltrans.o: In function `foo':
> > cce6KuRb.ltrans0.o:(.text+0x28): undefined reference to `sqrt'
> > 
> > because nothing links against libm.  I'm a bit confused by the comment
> > above: even with the linker plugin, where should libm be pulled in?
> > Don't we need to use -lm on the linker line anyway?
> 
> Well, the testcase was for an ICE which required that cabs is folded
> to sqrt(real**2 + imag**2), it requires "late" folding, thus
> -O0 during the compile step (it's a trick to use LTO here, to delay
> folding until after IPA opts).  Not linking with -lm and relying
> on a HW sqrt instruction makes sure we do that late folding
> which we want to happen.
> 
> So, if you can in any way retain all of the above features with
> fixing non-HW sqrt targets fine - otherwise I suggest you skip spu-elf.

Well, it works for me with just adding -lm to the dg-extra-ld-options.
This still folds cabs to sqrt in the LTO step, and then satisfies that
call via the libm routine ...  If I understood your intent correctly,
this should still test the same thing, shouldn't it?

Bye,
Ulrich

ChangeLog:

	* testsuite/gcc.dg/lto/20110201-1_0.c: Add -lm to dg-extra-ld-options.
	Update comment.

Index: gcc/testsuite/gcc.dg/lto/20110201-1_0.c
===================================================================
*** gcc/testsuite/gcc.dg/lto/20110201-1_0.c	(revision 176572)
--- gcc/testsuite/gcc.dg/lto/20110201-1_0.c	(working copy)
***************
*** 1,12 ****
  /* { dg-lto-do run } */
  /* { dg-lto-options { { -O0 -flto } } } */
! /* { dg-extra-ld-options "-O2 -ffast-math -fuse-linker-plugin" } */
  /* { dg-require-linker-plugin "" } */
  
! /* We require a linker plugin because otherwise we'd need to link
!    against libm which we are not sure here has cabs on all targets.
!    This is because collect2 invokes ld on the -O0 object code
!    which does not have folded cabs.  */
  
  double cabs(_Complex double);
  double __attribute__((used))
--- 1,14 ----
  /* { dg-lto-do run } */
  /* { dg-lto-options { { -O0 -flto } } } */
! /* { dg-extra-ld-options "-O2 -ffast-math -fuse-linker-plugin -lm" } */
  /* { dg-require-linker-plugin "" } */
  
! /* Since we compile with -O0, the object code will have a reference
!    to cabs.  The LTO link step will then fold this to either a call
!    to sqrt or a hardware square-root instruction.  We require the
!    linker plugin because otherwise a reference to cabs might remain
!    in the final output, which is not present on all targets.  We
!    still link against libm in case it is needed for sqrt.  */
  
  double cabs(_Complex double);
  double __attribute__((used))

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] Fix PR47566

[Richard Henderson]

On 07/22/2011 07:42 AM, Ulrich Weigand wrote:
> Well, it works for me with just adding -lm to the dg-extra-ld-options.
> This still folds cabs to sqrt in the LTO step, and then satisfies that
> call via the libm routine ...  If I understood your intent correctly,
> this should still test the same thing, shouldn't it?

Not quite, since -lm also provides cabs.


r~

Re: [PATCH] Fix PR47566

[Ulrich Weigand]

Richard Henderson wrote:
> On 07/22/2011 07:42 AM, Ulrich Weigand wrote:
> > Well, it works for me with just adding -lm to the dg-extra-ld-options.
> > This still folds cabs to sqrt in the LTO step, and then satisfies that
> > call via the libm routine ...  If I understood your intent correctly,
> > this should still test the same thing, shouldn't it?
> 
> Not quite, since -lm also provides cabs.

Well, yes (mine does as well).  But the -O2 LTO step will still fold cabs
to sqrt instead of pulling in the cabs from the library, at least it does
for me.  Thus the test still verifies this folding doesn't crash, which
is what I understood it should do.  Am I missing something?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] Fix PR47566

[Richard Henderson]

On 07/22/2011 10:19 AM, Ulrich Weigand wrote:
> Richard Henderson wrote:
>> On 07/22/2011 07:42 AM, Ulrich Weigand wrote:
>>> Well, it works for me with just adding -lm to the dg-extra-ld-options.
>>> This still folds cabs to sqrt in the LTO step, and then satisfies that
>>> call via the libm routine ...  If I understood your intent correctly,
>>> this should still test the same thing, shouldn't it?
>>
>> Not quite, since -lm also provides cabs.
> 
> Well, yes (mine does as well).  But the -O2 LTO step will still fold cabs
> to sqrt instead of pulling in the cabs from the library, at least it does
> for me.  Thus the test still verifies this folding doesn't crash, which
> is what I understood it should do.  Am I missing something?

Oops, sorry.  I forgot we were testing for a crash, not verifying the fold.


r~

Re: [PATCH] Fix PR47566

[Richard Guenther]

On Fri, Jul 22, 2011 at 7:28 PM, Richard Henderson <rth@redhat.com> wrote:
> On 07/22/2011 10:19 AM, Ulrich Weigand wrote:
>> Richard Henderson wrote:
>>> On 07/22/2011 07:42 AM, Ulrich Weigand wrote:
>>>> Well, it works for me with just adding -lm to the dg-extra-ld-options.
>>>> This still folds cabs to sqrt in the LTO step, and then satisfies that
>>>> call via the libm routine ...  If I understood your intent correctly,
>>>> this should still test the same thing, shouldn't it?
>>>
>>> Not quite, since -lm also provides cabs.
>>
>> Well, yes (mine does as well).  But the -O2 LTO step will still fold cabs
>> to sqrt instead of pulling in the cabs from the library, at least it does
>> for me.  Thus the test still verifies this folding doesn't crash, which
>> is what I understood it should do.  Am I missing something?
>
> Oops, sorry.  I forgot we were testing for a crash, not verifying the fold.

Of course testing for the fold makes sure we also test for the crash ...
so yes, the idea was to also test for the fold.  As the LTO testing
harness does not support scanning dumps (sigh...) link tests are
the only possibility right now.

Richard.

>
> r~
>

Re: [PATCH] Fix PR47566

[Ulrich Weigand]

Richard Guenther wrote:

> Of course testing for the fold makes sure we also test for the crash ...
> so yes, the idea was to also test for the fold.  As the LTO testing
> harness does not support scanning dumps (sigh...) link tests are
> the only possibility right now.

OK, so what about the following version?  This keeps not linking against
libm so it will fail if cabs is not folded.  On the other hand, it will
provide a dummy implementation of sqrt to avoid failure simply because
sqrt is not expanded inline ...

Bye,
Ulrich


ChangeLog:

	* testsuite/gcc.dg/lto/20110201-1_0.c: Provide dummy sqrt.

Index: gcc/testsuite/gcc.dg/lto/20110201-1_0.c
===================================================================
*** gcc/testsuite/gcc.dg/lto/20110201-1_0.c	(revision 177409)
--- gcc/testsuite/gcc.dg/lto/20110201-1_0.c	(working copy)
*************** foo (_Complex double x, int b)
*** 16,19 ****
--- 16,29 ----
      x = 0;
    return cabs(x);
  }
+ 
+ /* We provide a dummy sqrt to avoid link failures on targets that do not
+    expand sqrt inline.  Note that we do not link against libm in order
+    to ensure cabs is not satisfied by the library, but must be folded.  */
+ double __attribute__((used))
+ sqrt (double x)
+ {
+   return x;
+ }
+ 
  int main() { return 0; }


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] Fix PR47566

[Richard Guenther]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1622 bytes --]

On Mon, 8 Aug 2011, Ulrich Weigand wrote:

> Richard Guenther wrote:
> 
> > Of course testing for the fold makes sure we also test for the crash ...
> > so yes, the idea was to also test for the fold.  As the LTO testing
> > harness does not support scanning dumps (sigh...) link tests are
> > the only possibility right now.
> 
> OK, so what about the following version?  This keeps not linking against
> libm so it will fail if cabs is not folded.  On the other hand, it will
> provide a dummy implementation of sqrt to avoid failure simply because
> sqrt is not expanded inline ...

Looks good to me.

Richard.

> Bye,
> Ulrich
> 
> 
> ChangeLog:
> 
> 	* testsuite/gcc.dg/lto/20110201-1_0.c: Provide dummy sqrt.
> 
> Index: gcc/testsuite/gcc.dg/lto/20110201-1_0.c
> ===================================================================
> *** gcc/testsuite/gcc.dg/lto/20110201-1_0.c	(revision 177409)
> --- gcc/testsuite/gcc.dg/lto/20110201-1_0.c	(working copy)
> *************** foo (_Complex double x, int b)
> *** 16,19 ****
> --- 16,29 ----
>       x = 0;
>     return cabs(x);
>   }
> + 
> + /* We provide a dummy sqrt to avoid link failures on targets that do not
> +    expand sqrt inline.  Note that we do not link against libm in order
> +    to ensure cabs is not satisfied by the library, but must be folded.  */
> + double __attribute__((used))
> + sqrt (double x)
> + {
> +   return x;
> + }
> + 
>   int main() { return 0; }
> 
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer

Re: [PATCH] Fix PR47566

[Ulrich Weigand]

Richard Guenther wrote:
> On Mon, 8 Aug 2011, Ulrich Weigand wrote:
> > OK, so what about the following version?  This keeps not linking against
> > libm so it will fail if cabs is not folded.  On the other hand, it will
> > provide a dummy implementation of sqrt to avoid failure simply because
> > sqrt is not expanded inline ...
> 
> Looks good to me.

Thanks, I've checked this in now.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com