PR tree-opt/33616: ICE on callee-copied constructor arguments

PR tree-opt/33616: ICE on callee-copied constructor arguments

[Richard Sandiford]

The following code:

---------------------------------------------------------------------------
typedef float V8SF __attribute__ ((vector_size (32)));
void test0 (V8SF);
void
foo (float x)
{
  test ((V8SF) { x, x, x, x, x, x, x, x });
}
---------------------------------------------------------------------------

causes an ICE on targets for which the vector argument is callee-copied:

---------------------------------------------------------------------------
/tmp/foo.c: In function 'foo':
/tmp/foo.c:6: internal compiler error: in copy_constant, at varasm.c:3067
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.
---------------------------------------------------------------------------

We try to take the address of the original CONSTRUCTOR (rather than
taking the address of a temporary as in the caller-copied case) and
expand_expr_addr_expr_1 assumes that all CONSTRUCTORs are constant:

  /* If we are taking the address of a constant and are at the top level,
     we have to use output_constant_def since we can't call force_const_mem
     at top level.  */
  /* ??? This should be considered a front-end bug.  We should not be
     generating ADDR_EXPR of something that isn't an LVALUE.  The only
     exception here is STRING_CST.  */
  if (TREE_CODE (exp) == CONSTRUCTOR
      || CONSTANT_CLASS_P (exp))
    return XEXP (expand_expr_constant (exp, 0, modifier), 0);

However, the comment doesn't really seem to fit this case:
the CONSTRUCTOR _is_ an lvalue.  In the post-gimple world,
all real constant CONSTRUCTORs (i.e. those that can be put
in a readonly data segment) will already have been turned
into decls, even for -O0.  And it seems perfectly valid to
take the address of the non-constant constructor above.

The patch therefore treats CONSTRUCTORs in the same way as
decls and language-specific codes, namely by passing them
through expand_expr.  This will yield a MEM for the
constructor contents.

(If the agreement is that it isn't in fact valid to take the address of
a constructor, we could instead check TREE_CODE (base) != CONSTRUCTOR in
the following calls.c code:

	  /* If we're compiling a thunk, pass through invisible references
	     instead of making a copy.  */
	  if (call_from_thunk_p
	      || (callee_copies
		  && !TREE_ADDRESSABLE (type)
		  && (base = get_base_address (args[i].tree_value))
		  && (!DECL_P (base) || MEM_P (DECL_RTL (base)))))

We'd then treat callee-copied constructors exactly like caller-copied ones.)

This patch fixes gcc.c-torture/compile/20050113-1.c for mips32-elf.
However, that test is really for something else, and because its
vector is only 8 bytes wide, it doesn't fail for mipsisa64-elf.
I've therefore added a new test specifically for this bug.
This version fails on both targets.

Bootstrapped & regression-tested on x86_64-linux-gnu.  Also
regression-tested on mipsisa32-elf.  OK to install?

Richard


gcc/
	PR middle-end/33616
	* expr.c (expand_expr_addr_expr_1): Pass CONSTRUCTORs to
	expand_expr.

gcc/testsuite/
	PR middle-end/33616
	* gcc.c-torture/compile/pr33616.c: New test.

Index: gcc/expr.c
===================================================================
--- gcc/expr.c	2007-10-01 23:48:26.000000000 +0100
+++ gcc/expr.c	2007-10-01 23:55:09.000000000 +0100
@@ -6740,8 +6740,7 @@ expand_expr_addr_expr_1 (tree exp, rtx t
   /* ??? This should be considered a front-end bug.  We should not be
      generating ADDR_EXPR of something that isn't an LVALUE.  The only
      exception here is STRING_CST.  */
-  if (TREE_CODE (exp) == CONSTRUCTOR
-      || CONSTANT_CLASS_P (exp))
+  if (CONSTANT_CLASS_P (exp))
     return XEXP (expand_expr_constant (exp, 0, modifier), 0);
 
   /* Everything must be something allowed by is_gimple_addressable.  */
@@ -6788,9 +6787,12 @@ expand_expr_addr_expr_1 (tree exp, rtx t
     default:
       /* If the object is a DECL, then expand it for its rtl.  Don't bypass
 	 expand_expr, as that can have various side effects; LABEL_DECLs for
-	 example, may not have their DECL_RTL set yet.  Assume language
-	 specific tree nodes can be expanded in some interesting way.  */
+	 example, may not have their DECL_RTL set yet.  Expand the rtl of
+	 CONSTRUCTORs too, which should yield a memory reference for the
+	 constructor's contents.  Assume language specific tree nodes can
+	 be expanded in some interesting way.  */
       if (DECL_P (exp)
+	  || TREE_CODE (exp) == CONSTRUCTOR
 	  || TREE_CODE (exp) >= LAST_AND_UNUSED_TREE_CODE)
 	{
 	  result = expand_expr (exp, target, tmode,
Index: gcc/testsuite/gcc.c-torture/compile/pr33616.c
===================================================================
--- /dev/null	2007-09-27 09:37:00.556097250 +0100
+++ gcc/testsuite/gcc.c-torture/compile/pr33616.c	2007-10-01 23:48:52.000000000 +0100
@@ -0,0 +1,7 @@
+typedef float V8SF __attribute__ ((vector_size (32)));
+void test0 (V8SF);
+void
+foo (float x)
+{
+  test ((V8SF) { x, x, x, x, x, x, x, x });
+}

Re: PR tree-opt/33616: ICE on callee-copied constructor arguments

[Richard Sandiford]

Sorry, this is actually PR 33617.  I filed three bugs at once and
forgot to check whether they got consecutive numbers.  I've updated
the changelog and testcase accordingly.

Richard

Re: PR tree-opt/33616: ICE on callee-copied constructor arguments

[Richard Guenther]

On 10/2/07, Richard Sandiford <rsandifo@nildram.co.uk> wrote:
> The following code:
>
> ---------------------------------------------------------------------------
> typedef float V8SF __attribute__ ((vector_size (32)));
> void test0 (V8SF);
> void
> foo (float x)
> {
>   test ((V8SF) { x, x, x, x, x, x, x, x });
> }
> ---------------------------------------------------------------------------
>
> causes an ICE on targets for which the vector argument is callee-copied:
>
> ---------------------------------------------------------------------------
> /tmp/foo.c: In function 'foo':
> /tmp/foo.c:6: internal compiler error: in copy_constant, at varasm.c:3067
> Please submit a full bug report,
> with preprocessed source if appropriate.
> See <http://gcc.gnu.org/bugs.html> for instructions.
> ---------------------------------------------------------------------------
>
> We try to take the address of the original CONSTRUCTOR (rather than
> taking the address of a temporary as in the caller-copied case) and
> expand_expr_addr_expr_1 assumes that all CONSTRUCTORs are constant:
>
>   /* If we are taking the address of a constant and are at the top level,
>      we have to use output_constant_def since we can't call force_const_mem
>      at top level.  */
>   /* ??? This should be considered a front-end bug.  We should not be
>      generating ADDR_EXPR of something that isn't an LVALUE.  The only
>      exception here is STRING_CST.  */
>   if (TREE_CODE (exp) == CONSTRUCTOR
>       || CONSTANT_CLASS_P (exp))
>     return XEXP (expand_expr_constant (exp, 0, modifier), 0);
>
> However, the comment doesn't really seem to fit this case:
> the CONSTRUCTOR _is_ an lvalue.  In the post-gimple world,
> all real constant CONSTRUCTORs (i.e. those that can be put
> in a readonly data segment) will already have been turned
> into decls, even for -O0.  And it seems perfectly valid to
> take the address of the non-constant constructor above.
>
> The patch therefore treats CONSTRUCTORs in the same way as
> decls and language-specific codes, namely by passing them
> through expand_expr.  This will yield a MEM for the
> constructor contents.
>
> (If the agreement is that it isn't in fact valid to take the address of
> a constructor, we could instead check TREE_CODE (base) != CONSTRUCTOR in
> the following calls.c code:
>
>           /* If we're compiling a thunk, pass through invisible references
>              instead of making a copy.  */
>           if (call_from_thunk_p
>               || (callee_copies
>                   && !TREE_ADDRESSABLE (type)
>                   && (base = get_base_address (args[i].tree_value))
>                   && (!DECL_P (base) || MEM_P (DECL_RTL (base)))))
>
> We'd then treat callee-copied constructors exactly like caller-copied ones.)
>
> This patch fixes gcc.c-torture/compile/20050113-1.c for mips32-elf.
> However, that test is really for something else, and because its
> vector is only 8 bytes wide, it doesn't fail for mipsisa64-elf.
> I've therefore added a new test specifically for this bug.
> This version fails on both targets.
>
> Bootstrapped & regression-tested on x86_64-linux-gnu.  Also
> regression-tested on mipsisa32-elf.  OK to install?

Ok.  I wonder if ...

> +        constructor's contents.  Assume language specific tree nodes can
> +        be expanded in some interesting way.  */
>        if (DECL_P (exp)
> +         || TREE_CODE (exp) == CONSTRUCTOR
>           || TREE_CODE (exp) >= LAST_AND_UNUSED_TREE_CODE)
>         {

... we ever end up with lang-specific tree nodes here.  If you feel
eager to try,
a patch to remove that case is pre-approved if it passes bootstrap & regtest.

Thanks,
Richard.

Re: PR tree-opt/33616: ICE on callee-copied constructor arguments

[Eric Botcazou]

> We try to take the address of the original CONSTRUCTOR (rather than
> taking the address of a temporary as in the caller-copied case) and
> expand_expr_addr_expr_1 assumes that all CONSTRUCTORs are constant:
>
>   /* If we are taking the address of a constant and are at the top level,
>      we have to use output_constant_def since we can't call force_const_mem
>      at top level.  */
>   /* ??? This should be considered a front-end bug.  We should not be
>      generating ADDR_EXPR of something that isn't an LVALUE.  The only
>      exception here is STRING_CST.  */
>   if (TREE_CODE (exp) == CONSTRUCTOR
>
>       || CONSTANT_CLASS_P (exp))
>
>     return XEXP (expand_expr_constant (exp, 0, modifier), 0);
>
> However, the comment doesn't really seem to fit this case:
> the CONSTRUCTOR _is_ an lvalue.

No, a CONSTRUCTOR is not an lvalue, it cannot be the LHS of an assignment.

> In the post-gimple world, all real constant CONSTRUCTORs (i.e. those that
> can be put in a readonly data segment) will already have been turned
> into decls, even for -O0.  And it seems perfectly valid to
> take the address of the non-constant constructor above.

Yes, taking the address of a CONSTRUCTOR is allowed and the effect is to 
create a temporary initialized with the contents of the constructor.

> The patch therefore treats CONSTRUCTORs in the same way as
> decls and language-specific codes, namely by passing them
> through expand_expr.  This will yield a MEM for the
> constructor contents.

I think that's OK because of initializer_constant_valid_p:

    case ADDR_EXPR:
    case FDESC_EXPR:
      value = staticp (TREE_OPERAND (value, 0));
      if (value)
	{
[...]
	  /* "&{...}" requires a temporary to hold the constructed
	     object.  */
	  if (TREE_CODE (value) == CONSTRUCTOR)
	    return NULL_TREE;
	}
      return value;

that is to say, expand_expr_addr_expr_1 is not supposed to be passed ADDR_EXPR 
of CONSTRUCTOR at top-level (any longer, this was allowed before).

And I think that you should remove the ??? comment

  /* ??? This should be considered a front-end bug.  We should not be
     generating ADDR_EXPR of something that isn't an LVALUE.  The only
     exception here is STRING_CST.  */

now that the blatant non-LVALUE case has been eliminated.

-- 
Eric Botcazou

Re: PR tree-opt/33616: ICE on callee-copied constructor arguments

[Richard Sandiford]

"Richard Guenther" <richard.guenther@gmail.com> writes:
> On 10/2/07, Richard Sandiford <rsandifo@nildram.co.uk> wrote:
>> This patch fixes gcc.c-torture/compile/20050113-1.c for mips32-elf.
>> However, that test is really for something else, and because its
>> vector is only 8 bytes wide, it doesn't fail for mipsisa64-elf.
>> I've therefore added a new test specifically for this bug.
>> This version fails on both targets.
>>
>> Bootstrapped & regression-tested on x86_64-linux-gnu.  Also
>> regression-tested on mipsisa32-elf.  OK to install?
>
> Ok.

Thanks.

> I wonder if ...
>
>> +        constructor's contents.  Assume language specific tree nodes can
>> +        be expanded in some interesting way.  */
>>        if (DECL_P (exp)
>> +         || TREE_CODE (exp) == CONSTRUCTOR
>>           || TREE_CODE (exp) >= LAST_AND_UNUSED_TREE_CODE)
>>         {
>
> ... we ever end up with lang-specific tree nodes here.  If you feel
> eager to try,
> a patch to remove that case is pre-approved if it passes bootstrap & regtest.

Good question.  Sorry to be a wuss, but I'm not feeling that brave
right now...

Richard

Re: PR tree-opt/33616: ICE on callee-copied constructor arguments

[Richard Sandiford]

Eric Botcazou <ebotcazou@libertysurf.fr> writes:
>> We try to take the address of the original CONSTRUCTOR (rather than
>> taking the address of a temporary as in the caller-copied case) and
>> expand_expr_addr_expr_1 assumes that all CONSTRUCTORs are constant:
>>
>>   /* If we are taking the address of a constant and are at the top level,
>>      we have to use output_constant_def since we can't call force_const_mem
>>      at top level.  */
>>   /* ??? This should be considered a front-end bug.  We should not be
>>      generating ADDR_EXPR of something that isn't an LVALUE.  The only
>>      exception here is STRING_CST.  */
>>   if (TREE_CODE (exp) == CONSTRUCTOR
>>
>>       || CONSTANT_CLASS_P (exp))
>>
>>     return XEXP (expand_expr_constant (exp, 0, modifier), 0);
>>
>> However, the comment doesn't really seem to fit this case:
>> the CONSTRUCTOR _is_ an lvalue.
>
> No, a CONSTRUCTOR is not an lvalue, it cannot be the LHS of an assignment.

Sorry, my mistake.  I'd assumed that because C's COMPOUND_LITERAL_EXPRs
are lvalues -- as in:

    struct s { int i, j; };
    void foo (void) { (struct s) { 1, 2 } = (struct s) { 3, 4 }; }

-- that CONSTRUCTORs would be too.

> And I think that you should remove the ??? comment
>
>   /* ??? This should be considered a front-end bug.  We should not be
>      generating ADDR_EXPR of something that isn't an LVALUE.  The only
>      exception here is STRING_CST.  */
>
> now that the blatant non-LVALUE case has been eliminated.

Well, the comment is still above a check for CONSTANT_CLASS_P:

  /* If we are taking the address of a constant and are at the top level,
     we have to use output_constant_def since we can't call force_const_mem
     at top level.  */
  /* ??? This should be considered a front-end bug.  We should not be
     generating ADDR_EXPR of something that isn't an LVALUE.  The only
     exception here is STRING_CST.  */
  if (CONSTANT_CLASS_P (exp))
    return XEXP (expand_expr_constant (exp, 0, modifier), 0);

and if I were reading that without the ???, I'd be wondering why we're
taking tha address of things like INTEGER_CSTs.  I think it's useful
to have a comment that says we aren't really doing so out of choice.

Richard

Re: PR tree-opt/33616: ICE on callee-copied constructor arguments

[Joseph S. Myers]

On Tue, 2 Oct 2007, Richard Sandiford wrote:

> Sorry, my mistake.  I'd assumed that because C's COMPOUND_LITERAL_EXPRs
> are lvalues -- as in:
> 
>     struct s { int i, j; };
>     void foo (void) { (struct s) { 1, 2 } = (struct s) { 3, 4 }; }
> 
> -- that CONSTRUCTORs would be too.

And specifically, COMPOUND_LITERAL_EXPR was created because CONSTRUCTORs 
are not lvalues and otherwise do not satisfy the semantics of compound 
literals.

http://gcc.gnu.org/ml/gcc/2001-11/msg01301.html

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: PR tree-opt/33616: ICE on callee-copied constructor arguments

[Eric Botcazou]

> Well, the comment is still above a check for CONSTANT_CLASS_P:
>
>   /* If we are taking the address of a constant and are at the top level,
>      we have to use output_constant_def since we can't call force_const_mem
>      at top level.  */
>   /* ??? This should be considered a front-end bug.  We should not be
>      generating ADDR_EXPR of something that isn't an LVALUE.  The only
>      exception here is STRING_CST.  */
>   if (CONSTANT_CLASS_P (exp))
>     return XEXP (expand_expr_constant (exp, 0, modifier), 0);
>
> and if I were reading that without the ???, I'd be wondering why we're
> taking tha address of things like INTEGER_CSTs.  I think it's useful
> to have a comment that says we aren't really doing so out of choice.

My understanding is that the comment was primarly aimed at CONSTRUCTORs, 
because I doubt that we take the address of INTEGER_CSTs, but OK.

-- 
Eric Botcazou

Re: PR tree-opt/33616: ICE on callee-copied constructor arguments

[Richard Kenner]

> My understanding is that the comment was primarly aimed at CONSTRUCTORs, 
> because I doubt that we take the address of INTEGER_CSTs, but OK.

We used to.  Both Ada and Fortran would ...

Re: PR tree-opt/33616: ICE on callee-copied constructor arguments

[Eric Botcazou]

> We used to.  Both Ada and Fortran would ...

OK, I should have done my homework. ;-)

-- 
Eric Botcazou