Re: [PATCH] Do not emit SAVE_EXPR for already assigned SSA_NAMEs (PR71606).

[Richard Biener]

On July 8, 2016 4:23:31 PM GMT+02:00, "Martin Liška"  wrote:
>On 07/07/2016 04:15 PM, Richard Biener wrote:
>> I think it's fine though the inliners initializer handling looks
>> incredibly fragile to me ;)
>> 
>> Richard.
>
>OK, installed in trunk. May I install the patch to all active branches?
>Reg works for all of them.

Sure.

Richard.

Re: [PATCH] Do not emit SAVE_EXPR for already assigned SSA_NAMEs (PR71606).

[Martin Liška]

On 07/07/2016 04:15 PM, Richard Biener wrote:
> I think it's fine though the inliners initializer handling looks
> incredibly fragile to me ;)
> 
> Richard.

OK, installed in trunk. May I install the patch to all active branches?
Reg works for all of them.

Re: [PATCH] Do not emit SAVE_EXPR for already assigned SSA_NAMEs (PR71606).

[Richard Biener]

On Thu, Jul 7, 2016 at 4:01 PM, Martin Liška  wrote:
> On 07/01/2016 12:15 PM, Richard Biener wrote:
>> IMHO using fold-convert in this case is bogus and ideally the testcase
>> should have been diagnosed.
>>
>> fold_convertible_p has a comment
>>
>> /* Returns true, if ARG is convertible to TYPE using a NOP_EXPR.  *
>>
>> but clearly it isn't generating just a NOP_EXPR (or VIEW_CONVERT_EXPR
>> or other single operation) here.
>>
>> So that is the thing to fix.  The way we build / insert the init stmts
>> can also be improved by properly
>> gimplifying the rhs first but of course that likely runs into the
>> SAVE_EXPR case you mentioned.
>>
>> Richard.
>
> Hello Richard.
>
> I've tried to mark COMPLEX_TYPE as not acceptable by fold_convertible_p,
> which fixes the ICE and regression and bootstrap on x86_64-linux-gnu also 
> looks
> fine.
>
> Is it sufficient, or we would need more sophisticated approach to handle the 
> PR?

I think it's fine though the inliners initializer handling looks
incredibly fragile to me ;)

Richard.

> Thanks,
> Martin

Re: [PATCH] Do not emit SAVE_EXPR for already assigned SSA_NAMEs (PR71606).

[Martin Liška]

On 07/01/2016 12:15 PM, Richard Biener wrote:
> IMHO using fold-convert in this case is bogus and ideally the testcase
> should have been diagnosed.
> 
> fold_convertible_p has a comment
> 
> /* Returns true, if ARG is convertible to TYPE using a NOP_EXPR.  *
> 
> but clearly it isn't generating just a NOP_EXPR (or VIEW_CONVERT_EXPR
> or other single operation) here.
> 
> So that is the thing to fix.  The way we build / insert the init stmts
> can also be improved by properly
> gimplifying the rhs first but of course that likely runs into the
> SAVE_EXPR case you mentioned.
> 
> Richard.

Hello Richard.

I've tried to mark COMPLEX_TYPE as not acceptable by fold_convertible_p,
which fixes the ICE and regression and bootstrap on x86_64-linux-gnu also looks
fine.

Is it sufficient, or we would need more sophisticated approach to handle the PR?

Thanks,
Martin
>From 9c11a34d262bccd2185f2cdcaa3b9db420366073 Mon Sep 17 00:00:00 2001
From: marxin 
Date: Wed, 22 Jun 2016 18:07:55 +0200
Subject: [PATCH] Do not consider COMPLEX_TYPE as fold_convertible_p

gcc/ChangeLog:

2016-06-22  Martin Liska  

	PR middle-end/71606
	* fold-const.c (fold_convertible_p): As COMPLEX_TYPE
	folding produces SAVE_EXPRs, thus return false for the type.

gcc/testsuite/ChangeLog:

2016-06-22  Martin Liska  

	* gcc.dg/torture/pr71606.c: New test.
---
 gcc/fold-const.c   |  1 -
 gcc/testsuite/gcc.dg/torture/pr71606.c | 11 +++
 2 files changed, 11 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr71606.c

diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 3b9500d..f5d634e 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -2192,7 +2192,6 @@ fold_convertible_p (const_tree type, const_tree arg)
 
 case REAL_TYPE:
 case FIXED_POINT_TYPE:
-case COMPLEX_TYPE:
 case VECTOR_TYPE:
 case VOID_TYPE:
   return TREE_CODE (type) == TREE_CODE (orig);
diff --git a/gcc/testsuite/gcc.dg/torture/pr71606.c b/gcc/testsuite/gcc.dg/torture/pr71606.c
new file mode 100644
index 000..b0cc26a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr71606.c
@@ -0,0 +1,11 @@
+_Complex a;
+void fn1 ();
+
+int main () {
+  fn1 (a);
+  return 0;
+}
+
+void fn1 (__complex__ long double p1) {
+  __imag__ p1 = 6.0L;
+}
-- 
2.8.4

Re: [PATCH] Do not emit SAVE_EXPR for already assigned SSA_NAMEs (PR71606).

[Richard Biener]

On Thu, Jun 23, 2016 at 1:14 PM, Eric Botcazou  wrote:
>> The gimplifier has been changed recently to use anonymous SSA_NAMEs instead
>> of temporary decls.
>
> But the PR is a regression present since GCC 4.7...
>
>> And the gimplifier uses save_expr (which is a gimplifier function BTW) on
>> both not gimplified at all as well as partially gimplified trees.
>
> Are you confounding it with something else?  Because save_expr is definitely
> not a gimplifier function, it's mostly used to build GENERIC trees.  That
> being said, I can imagine it being invoked from the gimplifier, but I'd like
> to see the backtrace.

So the issue is that the inliner uses fold_convert:

  if (value
  && value != error_mark_node
  && !useless_type_conversion_p (TREE_TYPE (p), TREE_TYPE (value)))
{
  /* If we can match up types by promotion/demotion do so.  */
  if (fold_convertible_p (TREE_TYPE (p), value))
rhs = fold_convert (TREE_TYPE (p), value);
  else

and converting a _Complex double to a _Complex long double.  That
generates

COMPLEX_EXPR <(long double) REALPART_EXPR >, (long
double) IMAGPART_EXPR >>

which in insert_init_stmt we insert into the IL w/o gimplifying it
(thus the operand
scanner ICEs).

IMHO using fold-convert in this case is bogus and ideally the testcase
should have been diagnosed.

fold_convertible_p has a comment

/* Returns true, if ARG is convertible to TYPE using a NOP_EXPR.  *

but clearly it isn't generating just a NOP_EXPR (or VIEW_CONVERT_EXPR
or other single operation) here.

So that is the thing to fix.  The way we build / insert the init stmts
can also be improved by properly
gimplifying the rhs first but of course that likely runs into the
SAVE_EXPR case you mentioned.

Richard.

> --
> Eric Botcazou

Re: [PATCH] Do not emit SAVE_EXPR for already assigned SSA_NAMEs (PR71606).

[Eric Botcazou]

> The gimplifier has been changed recently to use anonymous SSA_NAMEs instead
> of temporary decls.

But the PR is a regression present since GCC 4.7...

> And the gimplifier uses save_expr (which is a gimplifier function BTW) on
> both not gimplified at all as well as partially gimplified trees.

Are you confounding it with something else?  Because save_expr is definitely 
not a gimplifier function, it's mostly used to build GENERIC trees.  That 
being said, I can imagine it being invoked from the gimplifier, but I'd like 
to see the backtrace.

-- 
Eric Botcazou

Re: [PATCH] Do not emit SAVE_EXPR for already assigned SSA_NAMEs (PR71606).

[Jakub Jelinek]

On Thu, Jun 23, 2016 at 12:41:53PM +0200, Eric Botcazou wrote:
> > This is candidate patch for the PR, which do not create SAVE_EXPR trees for
> > already assigned SSA_NAMEs.
> > 
> > Patch survives reg on x86_64-linux-gnu.
> > 
> > Thoughts?
> 
> This looks like a layering violation, save_expr is a GENERIC thing so 
> invoking 
> it on an SSA_NAME is weird.  How does this happen?

The gimplifier has been changed recently to use anonymous SSA_NAMEs instead
of temporary decls.  And the gimplifier uses save_expr (which is a
gimplifier function BTW) on both not gimplified at all as well as partially
gimplified trees.

Jakub

Re: [PATCH] Do not emit SAVE_EXPR for already assigned SSA_NAMEs (PR71606).

[Eric Botcazou]

> This is candidate patch for the PR, which do not create SAVE_EXPR trees for
> already assigned SSA_NAMEs.
> 
> Patch survives reg on x86_64-linux-gnu.
> 
> Thoughts?

This looks like a layering violation, save_expr is a GENERIC thing so invoking 
it on an SSA_NAME is weird.  How does this happen?

-- 
Eric Botcazou