* PR tree-opt/33616: ICE on callee-copied constructor arguments
@ 2007-10-01 22:59 Richard Sandiford
2007-10-01 23:27 ` Richard Sandiford
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Richard Sandiford @ 2007-10-01 22:59 UTC (permalink / raw)
To: gcc-patches
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 });
+}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PR tree-opt/33616: ICE on callee-copied constructor arguments
2007-10-01 22:59 PR tree-opt/33616: ICE on callee-copied constructor arguments Richard Sandiford
@ 2007-10-01 23:27 ` Richard Sandiford
2007-10-02 9:47 ` Richard Guenther
2007-10-02 12:52 ` Eric Botcazou
2 siblings, 0 replies; 10+ messages in thread
From: Richard Sandiford @ 2007-10-01 23:27 UTC (permalink / raw)
To: gcc-patches
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PR tree-opt/33616: ICE on callee-copied constructor arguments
2007-10-01 22:59 PR tree-opt/33616: ICE on callee-copied constructor arguments Richard Sandiford
2007-10-01 23:27 ` Richard Sandiford
@ 2007-10-02 9:47 ` Richard Guenther
2007-10-02 15:53 ` Richard Sandiford
2007-10-02 12:52 ` Eric Botcazou
2 siblings, 1 reply; 10+ messages in thread
From: Richard Guenther @ 2007-10-02 9:47 UTC (permalink / raw)
To: gcc-patches, rsandifo
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PR tree-opt/33616: ICE on callee-copied constructor arguments
2007-10-02 9:47 ` Richard Guenther
@ 2007-10-02 15:53 ` Richard Sandiford
0 siblings, 0 replies; 10+ messages in thread
From: Richard Sandiford @ 2007-10-02 15:53 UTC (permalink / raw)
To: Richard Guenther; +Cc: gcc-patches
"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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PR tree-opt/33616: ICE on callee-copied constructor arguments
2007-10-01 22:59 PR tree-opt/33616: ICE on callee-copied constructor arguments Richard Sandiford
2007-10-01 23:27 ` Richard Sandiford
2007-10-02 9:47 ` Richard Guenther
@ 2007-10-02 12:52 ` Eric Botcazou
2007-10-02 15:54 ` Richard Sandiford
2 siblings, 1 reply; 10+ messages in thread
From: Eric Botcazou @ 2007-10-02 12:52 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PR tree-opt/33616: ICE on callee-copied constructor arguments
2007-10-02 12:52 ` Eric Botcazou
@ 2007-10-02 15:54 ` Richard Sandiford
2007-10-02 16:11 ` Joseph S. Myers
2007-10-02 19:36 ` Eric Botcazou
0 siblings, 2 replies; 10+ messages in thread
From: Richard Sandiford @ 2007-10-02 15:54 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PR tree-opt/33616: ICE on callee-copied constructor arguments
2007-10-02 15:54 ` Richard Sandiford
@ 2007-10-02 16:11 ` Joseph S. Myers
2007-10-02 19:36 ` Eric Botcazou
1 sibling, 0 replies; 10+ messages in thread
From: Joseph S. Myers @ 2007-10-02 16:11 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Eric Botcazou, gcc-patches
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PR tree-opt/33616: ICE on callee-copied constructor arguments
2007-10-02 15:54 ` Richard Sandiford
2007-10-02 16:11 ` Joseph S. Myers
@ 2007-10-02 19:36 ` Eric Botcazou
2007-10-02 19:52 ` Richard Kenner
1 sibling, 1 reply; 10+ messages in thread
From: Eric Botcazou @ 2007-10-02 19:36 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2007-10-02 20:21 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-10-01 22:59 PR tree-opt/33616: ICE on callee-copied constructor arguments Richard Sandiford
2007-10-01 23:27 ` Richard Sandiford
2007-10-02 9:47 ` Richard Guenther
2007-10-02 15:53 ` Richard Sandiford
2007-10-02 12:52 ` Eric Botcazou
2007-10-02 15:54 ` Richard Sandiford
2007-10-02 16:11 ` Joseph S. Myers
2007-10-02 19:36 ` Eric Botcazou
2007-10-02 19:52 ` Richard Kenner
2007-10-02 20:21 ` Eric Botcazou
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).