* [PATCH][PR65818][bootstrap,hppa] Return side-effect free result in gimplify_va_arg_internal
@ 2015-04-25 12:35 Tom de Vries
2015-04-27 8:17 ` Richard Biener
0 siblings, 1 reply; 8+ messages in thread
From: Tom de Vries @ 2015-04-25 12:35 UTC (permalink / raw)
To: GCC Patches; +Cc: John David Anglin
[-- Attachment #1: Type: text/plain, Size: 1940 bytes --]
Hi,
this patch fixes PR65818, and hppa bootstrap.
When compiling gcc/libiberty/vprintf-support.c, the following va_arg is compiled:
...
(void) __builtin_va_arg(ap, double);
...
This results in the following ifn_va_arg at gimple level, with a NULL_TREE lhs:
...
VA_ARG (&ap, 0B);
...
We start expanding the ifn_va_arg in expand_ifn_va_arg_1 by calling
gimplify_va_arg_internal:
...
expr = gimplify_va_arg_internal (ap, type, gimple_location (stmt),
&pre, &post);
...
Subsequently, because the lhs is NULL_TREE, we skip generating the assign to the
lhs:
...
lhs = gimple_call_lhs (stmt);
if (lhs != NULL_TREE)
{
gcc_assert (useless_type_conversion_p (TREE_TYPE (lhs), type));
if (gimple_call_num_args (stmt) == 3)
{
/* We've transported the size of with WITH_SIZE_EXPR here as
the 3rd argument of the internal fn call. Now reinstate
it. */
tree size = gimple_call_arg (stmt, 2);
expr = build2 (WITH_SIZE_EXPR, TREE_TYPE (expr), expr, size);
}
/* We use gimplify_assign here, rather than gimple_build_assign,
because gimple_assign knows how to deal with variable-sized
types. */
gimplify_assign (lhs, expr, &pre);
}
...
We assume here that any side-effects related to updating ap have been generated
into pre/post by gimplify_va_arg_internal, and that it's safe to ignore expr.
Turns out, that's not the case for hppa. The target hook
hppa_gimplify_va_arg_expr (called from gimplify_va_arg_internal) returns an
expression that still contains a side-effect:
...
*(double *) (ap = ap + 4294967288 & 4294967288B)
...
This patch fixes that by gimplifying the address expression of the mem-ref
returned by the target hook (borrowing code from gimplify_expr, case MEM_REF).
Bootstrapped and reg-tested on x86_64.
Bootstrapped and reg-tested on hppa2.0w-hp-hpux11.11.
OK for trunk?
Thanks,
- Tom
[-- Attachment #2: 0001-Return-side-effect-free-result-in-gimplify_va_arg_in.patch --]
[-- Type: text/x-patch, Size: 1061 bytes --]
Return side-effect free result in gimplify_va_arg_internal
2015-04-22 Tom de Vries <tom@codesourcery.com>
PR tree-optimization/65818
* gimplify.c (gimplify_va_arg_internal): Ensure that only side-effect
free values are returned.
---
gcc/gimplify.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index c68bd47..fe54e53 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -9352,7 +9352,12 @@ gimplify_va_arg_internal (tree valist, tree type, location_t loc,
else
gimplify_expr (&valist, pre_p, post_p, is_gimple_min_lval, fb_lvalue);
- return targetm.gimplify_va_arg_expr (valist, type, pre_p, post_p);
+ tree expr = targetm.gimplify_va_arg_expr (valist, type, pre_p, post_p);
+ gcc_assert (TREE_CODE (expr) == MEM_REF);
+ if (!is_gimple_mem_ref_addr (TREE_OPERAND (expr, 0)))
+ gimplify_expr (&TREE_OPERAND (expr, 0), pre_p, post_p,
+ is_gimple_mem_ref_addr, fb_rvalue);
+ return expr;
}
/* Gimplify __builtin_va_arg, aka VA_ARG_EXPR, which is not really a
--
1.9.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][PR65818][bootstrap,hppa] Return side-effect free result in gimplify_va_arg_internal
2015-04-25 12:35 [PATCH][PR65818][bootstrap,hppa] Return side-effect free result in gimplify_va_arg_internal Tom de Vries
@ 2015-04-27 8:17 ` Richard Biener
2015-04-27 13:06 ` Tom de Vries
0 siblings, 1 reply; 8+ messages in thread
From: Richard Biener @ 2015-04-27 8:17 UTC (permalink / raw)
To: Tom de Vries; +Cc: GCC Patches, John David Anglin
On Sat, Apr 25, 2015 at 2:35 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> Hi,
>
> this patch fixes PR65818, and hppa bootstrap.
>
> When compiling gcc/libiberty/vprintf-support.c, the following va_arg is
> compiled:
> ...
> (void) __builtin_va_arg(ap, double);
> ...
>
> This results in the following ifn_va_arg at gimple level, with a NULL_TREE
> lhs:
> ...
> VA_ARG (&ap, 0B);
> ...
>
> We start expanding the ifn_va_arg in expand_ifn_va_arg_1 by calling
> gimplify_va_arg_internal:
> ...
> expr = gimplify_va_arg_internal (ap, type, gimple_location (stmt),
> &pre, &post);
> ...
>
> Subsequently, because the lhs is NULL_TREE, we skip generating the assign to
> the lhs:
> ...
> lhs = gimple_call_lhs (stmt);
> if (lhs != NULL_TREE)
> {
> gcc_assert (useless_type_conversion_p (TREE_TYPE (lhs), type));
>
> if (gimple_call_num_args (stmt) == 3)
> {
> /* We've transported the size of with WITH_SIZE_EXPR here as
> the 3rd argument of the internal fn call. Now reinstate
> it. */
> tree size = gimple_call_arg (stmt, 2);
> expr = build2 (WITH_SIZE_EXPR, TREE_TYPE (expr), expr,
> size);
> }
>
> /* We use gimplify_assign here, rather than gimple_build_assign,
> because gimple_assign knows how to deal with variable-sized
> types. */
> gimplify_assign (lhs, expr, &pre);
> }
> ...
>
> We assume here that any side-effects related to updating ap have been
> generated into pre/post by gimplify_va_arg_internal, and that it's safe to
> ignore expr.
>
> Turns out, that's not the case for hppa. The target hook
> hppa_gimplify_va_arg_expr (called from gimplify_va_arg_internal) returns an
> expression that still contains a side-effect:
> ...
> *(double *) (ap = ap + 4294967288 & 4294967288B)
> ...
>
> This patch fixes that by gimplifying the address expression of the mem-ref
> returned by the target hook (borrowing code from gimplify_expr, case
> MEM_REF).
>
> Bootstrapped and reg-tested on x86_64.
>
> Bootstrapped and reg-tested on hppa2.0w-hp-hpux11.11.
>
> OK for trunk?
Hmm, that assert looks suspicious...
Can't you simply always do
gimplify_expr (expr, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
?
Richard.
> Thanks,
> - Tom
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][PR65818][bootstrap,hppa] Return side-effect free result in gimplify_va_arg_internal
2015-04-27 8:17 ` Richard Biener
@ 2015-04-27 13:06 ` Tom de Vries
2015-04-27 13:40 ` Richard Biener
0 siblings, 1 reply; 8+ messages in thread
From: Tom de Vries @ 2015-04-27 13:06 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches, John David Anglin
[-- Attachment #1: Type: text/plain, Size: 716 bytes --]
On 27-04-15 10:17, Richard Biener wrote:
>> This patch fixes that by gimplifying the address expression of the mem-ref
>> >returned by the target hook (borrowing code from gimplify_expr, case
>> >MEM_REF).
>> >
>> >Bootstrapped and reg-tested on x86_64.
>> >
>> >Bootstrapped and reg-tested on hppa2.0w-hp-hpux11.11.
>> >
>> >OK for trunk?
> Hmm, that assert looks suspicious...
>
> Can't you simply always do
>
> gimplify_expr (expr, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
It's a bit counter-intuitive for me, using is_gimple_lvalue for something (the
result of va_arg) we use as rvalue.
But, it seems to work (with & in front of expr).
OK if bootstrap and reg-test on x86_64 succeeds?
Thanks,
- Tom
[-- Attachment #2: 0001-Return-side-effect-free-result-in-gimplify_va_arg_in.patch --]
[-- Type: text/x-patch, Size: 922 bytes --]
Return side-effect free result in gimplify_va_arg_internal
2015-04-27 Tom de Vries <tom@codesourcery.com>
PR tree-optimization/65818
* gimplify.c (gimplify_va_arg_internal): Ensure that only side-effect
free values are returned.
---
gcc/gimplify.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index c68bd47..4a68c87 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -9352,7 +9352,9 @@ gimplify_va_arg_internal (tree valist, tree type, location_t loc,
else
gimplify_expr (&valist, pre_p, post_p, is_gimple_min_lval, fb_lvalue);
- return targetm.gimplify_va_arg_expr (valist, type, pre_p, post_p);
+ tree expr = targetm.gimplify_va_arg_expr (valist, type, pre_p, post_p);
+ gimplify_expr (&expr, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
+ return expr;
}
/* Gimplify __builtin_va_arg, aka VA_ARG_EXPR, which is not really a
--
1.9.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][PR65818][bootstrap,hppa] Return side-effect free result in gimplify_va_arg_internal
2015-04-27 13:06 ` Tom de Vries
@ 2015-04-27 13:40 ` Richard Biener
2015-04-27 15:04 ` Tom de Vries
0 siblings, 1 reply; 8+ messages in thread
From: Richard Biener @ 2015-04-27 13:40 UTC (permalink / raw)
To: Tom de Vries; +Cc: GCC Patches, John David Anglin
On Mon, Apr 27, 2015 at 3:06 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 27-04-15 10:17, Richard Biener wrote:
>>>
>>> This patch fixes that by gimplifying the address expression of the
>>> mem-ref
>>> >returned by the target hook (borrowing code from gimplify_expr, case
>>> >MEM_REF).
>>> >
>>> >Bootstrapped and reg-tested on x86_64.
>>> >
>>> >Bootstrapped and reg-tested on hppa2.0w-hp-hpux11.11.
>>> >
>>> >OK for trunk?
>>
>> Hmm, that assert looks suspicious...
>>
>> Can't you simply always do
>>
>> gimplify_expr (expr, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
>
>
> It's a bit counter-intuitive for me, using is_gimple_lvalue for something
> (the result of va_arg) we use as rvalue.
Yeah, choosing that was done because you could assert it's a MEM_REF
and tree-stdarg eventually builds a WITH_SIZE_EXPR around it.
It would possibly be easier to simply "inline" gimplify_va_arg_internal
at its use and only gimplify the assignment. Though in that case the
original code probably worked - just in the lhs == NULL case it didn't,
which hints at a better place for the fix - in expand_ifn_va_arg_1 do
if (lhs != NULL_TREE)
{
...
}
else
gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either);
So ... if you can re-try that one it's pre-approved.
Thanks,
Richard.
> But, it seems to work (with & in front of expr).
>
> OK if bootstrap and reg-test on x86_64 succeeds?
>
> Thanks,
> - Tom
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][PR65818][bootstrap,hppa] Return side-effect free result in gimplify_va_arg_internal
2015-04-27 13:40 ` Richard Biener
@ 2015-04-27 15:04 ` Tom de Vries
2015-04-28 10:49 ` Richard Biener
0 siblings, 1 reply; 8+ messages in thread
From: Tom de Vries @ 2015-04-27 15:04 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches, John David Anglin
[-- Attachment #1: Type: text/plain, Size: 4423 bytes --]
On 27-04-15 15:40, Richard Biener wrote:
> On Mon, Apr 27, 2015 at 3:06 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
>> On 27-04-15 10:17, Richard Biener wrote:
>>>>
>>>> This patch fixes that by gimplifying the address expression of the
>>>> mem-ref
>>>>> returned by the target hook (borrowing code from gimplify_expr, case
>>>>> MEM_REF).
>>>>>
>>>>> Bootstrapped and reg-tested on x86_64.
>>>>>
>>>>> Bootstrapped and reg-tested on hppa2.0w-hp-hpux11.11.
>>>>>
>>>>> OK for trunk?
>>>
>>> Hmm, that assert looks suspicious...
>>>
>>> Can't you simply always do
>>>
>>> gimplify_expr (expr, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
>>
>>
>> It's a bit counter-intuitive for me, using is_gimple_lvalue for something
>> (the result of va_arg) we use as rvalue.
>
> Yeah, choosing that was done because you could assert it's a MEM_REF
> and tree-stdarg eventually builds a WITH_SIZE_EXPR around it.
>
> It would possibly be easier to simply "inline" gimplify_va_arg_internal
> at its use and only gimplify the assignment. Though in that case the
> original code probably worked - just in the lhs == NULL case it didn't,
> which hints at a better place for the fix - in expand_ifn_va_arg_1 do
>
> if (lhs != NULL_TREE)
> {
> ...
> }
> else
> gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either);
>
> So ... if you can re-try that one it's pre-approved.
>
Using that proposed patch, we run into the following problem.
Consider this testcase with ignored-result-va_arg:
...
#include <stdarg.h>
void
f2 (int i, ...)
{
va_list ap;
va_start (ap, i);
va_arg (ap, long);
va_end (ap);
}
...
after gimplify_va_arg_internal we have:
...
(gdb) call debug_generic_expr (expr)
*(long int * {ref-all}) addr.2
...
In a bit more detail:
...
(gdb) call debug_tree (expr)
<mem_ref 0x7ffff65ea1b8
type <integer_type 0x7ffff64a77e0 long int DI
size <integer_cst 0x7ffff64a3ca8 constant 64>
unit size <integer_cst 0x7ffff64a3cc0 constant 8>
align 64 symtab 0 alias set -1 canonical type 0x7ffff64a77e0 precision
64 min <integer_cst 0x7ffff64a3f30 -9223372036854775808> max <integer_cst
0x7ffff64a3f48 9223372036854775807>
pointer_to_this <pointer_type 0x7ffff65e0498>>
arg 0 <nop_expr 0x7ffff65cbb60
type <pointer_type 0x7ffff65e0498 type <integer_type 0x7ffff64a77e0
long int>
public static unsigned DI size <integer_cst 0x7ffff64a3ca8 64> unit
size <integer_cst 0x7ffff64a3cc0 8>
align 64 symtab 0 alias set -1 canonical type 0x7ffff65e0498>
arg 0 <var_decl 0x7ffff65e32d0 addr.2 type <pointer_type 0x7ffff64c2150>
used unsigned ignored DI file stdarg-1.c line 4 col 1 size
<integer_cst 0x7ffff64a3ca8 64> unit size <integer_cst 0x7ffff64a3cc0 8>
align 64 context <function_decl 0x7ffff65c21b0 f2>>>
arg 1 <integer_cst 0x7ffff65e64e0 type <pointer_type 0x7ffff65e0498>
constant 0>>
...
During 'gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either)' we ICE here:
...
8886 gcc_assert ((*gimple_test_f) (*expr_p));
...
At this point expr is:
...
(gdb) call debug_generic_expr (*expr_p)
*addr.2
...
In more detail:
...
(gdb) call debug_tree (*expr_p)
<mem_ref 0x7ffff65ea1e0
type <void_type 0x7ffff64c2000 void VOID
align 8 symtab 0 alias set -1 canonical type 0x7ffff64c2000
pointer_to_this <pointer_type 0x7ffff64c2150>>
arg 0 <var_decl 0x7ffff65e32d0 addr.2
type <pointer_type 0x7ffff64c2150 type <void_type 0x7ffff64c2000 void>
sizes-gimplified public unsigned DI
size <integer_cst 0x7ffff64a3ca8 constant 64>
unit size <integer_cst 0x7ffff64a3cc0 constant 8>
align 64 symtab 0 alias set -1 canonical type 0x7ffff64c2150
pointer_to_this <pointer_type 0x7ffff64c9bd0>>
used unsigned ignored DI file stdarg-1.c line 4 col 1 size <integer_cst
0x7ffff64a3ca8 64> unit size <integer_cst 0x7ffff64a3cc0 8>
align 64 context <function_decl 0x7ffff65c21b0 f2>>
arg 1 <integer_cst 0x7ffff64c10c0 type <pointer_type 0x7ffff64c2150>
constant 0>>
...
The memref is now VOID type. And that's not a gimple_val:
...
(gdb) p is_gimple_val (*expr_p)
$1 = false
...
Attached patch uses is_gimple_lvalue, but in expand_ifn_va_arg_1.
I'll bootstrap and reg-test on x86_64 and commit, unless further comments of course.
Thanks,
- Tom
[-- Attachment #2: 0001-Evaluate-side-effects-in-expand_ifn_va_arg_1.patch --]
[-- Type: text/x-patch, Size: 645 bytes --]
Evaluate side-effects in expand_ifn_va_arg_1
2015-04-27 Tom de Vries <tom@codesourcery.com>
PR tree-optimization/65818
* tree-stdarg.c (expand_ifn_va_arg_1): Ensure that side-effects are
evaluated.
---
gcc/tree-stdarg.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/gcc/tree-stdarg.c b/gcc/tree-stdarg.c
index 16a9e2c..1356374 100644
--- a/gcc/tree-stdarg.c
+++ b/gcc/tree-stdarg.c
@@ -1078,6 +1078,8 @@ expand_ifn_va_arg_1 (function *fun)
types. */
gimplify_assign (lhs, expr, &pre);
}
+ else
+ gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
pop_gimplify_context (NULL);
--
1.9.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][PR65818][bootstrap,hppa] Return side-effect free result in gimplify_va_arg_internal
2015-04-27 15:04 ` Tom de Vries
@ 2015-04-28 10:49 ` Richard Biener
2015-04-28 21:59 ` Tom de Vries
0 siblings, 1 reply; 8+ messages in thread
From: Richard Biener @ 2015-04-28 10:49 UTC (permalink / raw)
To: Tom de Vries; +Cc: GCC Patches, John David Anglin
On Mon, Apr 27, 2015 at 5:04 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 27-04-15 15:40, Richard Biener wrote:
>>
>> On Mon, Apr 27, 2015 at 3:06 PM, Tom de Vries <Tom_deVries@mentor.com>
>> wrote:
>>>
>>> On 27-04-15 10:17, Richard Biener wrote:
>>>>>
>>>>>
>>>>> This patch fixes that by gimplifying the address expression of the
>>>>> mem-ref
>>>>>>
>>>>>> returned by the target hook (borrowing code from gimplify_expr, case
>>>>>> MEM_REF).
>>>>>>
>>>>>> Bootstrapped and reg-tested on x86_64.
>>>>>>
>>>>>> Bootstrapped and reg-tested on hppa2.0w-hp-hpux11.11.
>>>>>>
>>>>>> OK for trunk?
>>>>
>>>>
>>>> Hmm, that assert looks suspicious...
>>>>
>>>> Can't you simply always do
>>>>
>>>> gimplify_expr (expr, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
>>>
>>>
>>>
>>> It's a bit counter-intuitive for me, using is_gimple_lvalue for something
>>> (the result of va_arg) we use as rvalue.
>>
>>
>> Yeah, choosing that was done because you could assert it's a MEM_REF
>> and tree-stdarg eventually builds a WITH_SIZE_EXPR around it.
>>
>> It would possibly be easier to simply "inline" gimplify_va_arg_internal
>> at its use and only gimplify the assignment. Though in that case the
>> original code probably worked - just in the lhs == NULL case it didn't,
>> which hints at a better place for the fix - in expand_ifn_va_arg_1 do
>>
>> if (lhs != NULL_TREE)
>> {
>> ...
>> }
>> else
>> gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either);
>>
>> So ... if you can re-try that one it's pre-approved.
>>
>
> Using that proposed patch, we run into the following problem.
>
> Consider this testcase with ignored-result-va_arg:
> ...
> #include <stdarg.h>
>
> void
> f2 (int i, ...)
> {
> va_list ap;
> va_start (ap, i);
> va_arg (ap, long);
> va_end (ap);
> }
> ...
>
> after gimplify_va_arg_internal we have:
> ...
> (gdb) call debug_generic_expr (expr)
> *(long int * {ref-all}) addr.2
> ...
>
> In a bit more detail:
> ...
> (gdb) call debug_tree (expr)
> <mem_ref 0x7ffff65ea1b8
> type <integer_type 0x7ffff64a77e0 long int DI
> size <integer_cst 0x7ffff64a3ca8 constant 64>
> unit size <integer_cst 0x7ffff64a3cc0 constant 8>
> align 64 symtab 0 alias set -1 canonical type 0x7ffff64a77e0
> precision 64 min <integer_cst 0x7ffff64a3f30 -9223372036854775808> max
> <integer_cst 0x7ffff64a3f48 9223372036854775807>
> pointer_to_this <pointer_type 0x7ffff65e0498>>
>
> arg 0 <nop_expr 0x7ffff65cbb60
> type <pointer_type 0x7ffff65e0498 type <integer_type 0x7ffff64a77e0
> long int>
> public static unsigned DI size <integer_cst 0x7ffff64a3ca8 64>
> unit size <integer_cst 0x7ffff64a3cc0 8>
> align 64 symtab 0 alias set -1 canonical type 0x7ffff65e0498>
>
> arg 0 <var_decl 0x7ffff65e32d0 addr.2 type <pointer_type
> 0x7ffff64c2150>
> used unsigned ignored DI file stdarg-1.c line 4 col 1 size
> <integer_cst 0x7ffff64a3ca8 64> unit size <integer_cst 0x7ffff64a3cc0 8>
> align 64 context <function_decl 0x7ffff65c21b0 f2>>>
> arg 1 <integer_cst 0x7ffff65e64e0 type <pointer_type 0x7ffff65e0498>
> constant 0>>
> ...
>
> During 'gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either)' we ICE
> here:
> ...
> 8886 gcc_assert ((*gimple_test_f) (*expr_p));
> ...
>
> At this point expr is:
> ...
> (gdb) call debug_generic_expr (*expr_p)
> *addr.2
> ...
>
> In more detail:
> ...
> (gdb) call debug_tree (*expr_p)
> <mem_ref 0x7ffff65ea1e0
> type <void_type 0x7ffff64c2000 void VOID
> align 8 symtab 0 alias set -1 canonical type 0x7ffff64c2000
> pointer_to_this <pointer_type 0x7ffff64c2150>>
>
> arg 0 <var_decl 0x7ffff65e32d0 addr.2
> type <pointer_type 0x7ffff64c2150 type <void_type 0x7ffff64c2000
> void>
> sizes-gimplified public unsigned DI
> size <integer_cst 0x7ffff64a3ca8 constant 64>
> unit size <integer_cst 0x7ffff64a3cc0 constant 8>
> align 64 symtab 0 alias set -1 canonical type 0x7ffff64c2150
> pointer_to_this <pointer_type 0x7ffff64c9bd0>>
> used unsigned ignored DI file stdarg-1.c line 4 col 1 size
> <integer_cst 0x7ffff64a3ca8 64> unit size <integer_cst 0x7ffff64a3cc0 8>
> align 64 context <function_decl 0x7ffff65c21b0 f2>>
> arg 1 <integer_cst 0x7ffff64c10c0 type <pointer_type 0x7ffff64c2150>
> constant 0>>
> ...
>
> The memref is now VOID type. And that's not a gimple_val:
> ...
> (gdb) p is_gimple_val (*expr_p)
> $1 = false
> ...
On which target? I can't seem to reproduce on x86_64 or i?86. I also
can't see how this
could happen.
Richard.
> Attached patch uses is_gimple_lvalue, but in expand_ifn_va_arg_1.
>
> I'll bootstrap and reg-test on x86_64 and commit, unless further comments of
> course.
>
> Thanks,
> - Tom
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][PR65818][bootstrap,hppa] Return side-effect free result in gimplify_va_arg_internal
2015-04-28 10:49 ` Richard Biener
@ 2015-04-28 21:59 ` Tom de Vries
2015-04-29 9:53 ` Richard Biener
0 siblings, 1 reply; 8+ messages in thread
From: Tom de Vries @ 2015-04-28 21:59 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches, John David Anglin
[-- Attachment #1: Type: text/plain, Size: 5191 bytes --]
On 28-04-15 12:34, Richard Biener wrote:
> On Mon, Apr 27, 2015 at 5:04 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
>> On 27-04-15 15:40, Richard Biener wrote:
>>>
>>> On Mon, Apr 27, 2015 at 3:06 PM, Tom de Vries <Tom_deVries@mentor.com>
>>> wrote:
>>>>
>>>> On 27-04-15 10:17, Richard Biener wrote:
>>>>>>
>>>>>>
>>>>>> This patch fixes that by gimplifying the address expression of the
>>>>>> mem-ref
>>>>>>>
>>>>>>> returned by the target hook (borrowing code from gimplify_expr, case
>>>>>>> MEM_REF).
>>>>>>>
>>>>>>> Bootstrapped and reg-tested on x86_64.
>>>>>>>
>>>>>>> Bootstrapped and reg-tested on hppa2.0w-hp-hpux11.11.
>>>>>>>
>>>>>>> OK for trunk?
>>>>>
>>>>>
>>>>> Hmm, that assert looks suspicious...
>>>>>
>>>>> Can't you simply always do
>>>>>
>>>>> gimplify_expr (expr, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
>>>>
>>>>
>>>>
>>>> It's a bit counter-intuitive for me, using is_gimple_lvalue for something
>>>> (the result of va_arg) we use as rvalue.
>>>
>>>
>>> Yeah, choosing that was done because you could assert it's a MEM_REF
>>> and tree-stdarg eventually builds a WITH_SIZE_EXPR around it.
>>>
>>> It would possibly be easier to simply "inline" gimplify_va_arg_internal
>>> at its use and only gimplify the assignment. Though in that case the
>>> original code probably worked - just in the lhs == NULL case it didn't,
>>> which hints at a better place for the fix - in expand_ifn_va_arg_1 do
>>>
>>> if (lhs != NULL_TREE)
>>> {
>>> ...
>>> }
>>> else
>>> gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either);
>>>
>>> So ... if you can re-try that one it's pre-approved.
>>>
>>
>> Using that proposed patch, we run into the following problem.
>>
>> Consider this testcase with ignored-result-va_arg:
>> ...
>> #include <stdarg.h>
>>
>> void
>> f2 (int i, ...)
>> {
>> va_list ap;
>> va_start (ap, i);
>> va_arg (ap, long);
>> va_end (ap);
>> }
>> ...
>>
>> after gimplify_va_arg_internal we have:
>> ...
>> (gdb) call debug_generic_expr (expr)
>> *(long int * {ref-all}) addr.2
>> ...
>>
>> In a bit more detail:
>> ...
>> (gdb) call debug_tree (expr)
>> <mem_ref 0x7ffff65ea1b8
>> type <integer_type 0x7ffff64a77e0 long int DI
>> size <integer_cst 0x7ffff64a3ca8 constant 64>
>> unit size <integer_cst 0x7ffff64a3cc0 constant 8>
>> align 64 symtab 0 alias set -1 canonical type 0x7ffff64a77e0
>> precision 64 min <integer_cst 0x7ffff64a3f30 -9223372036854775808> max
>> <integer_cst 0x7ffff64a3f48 9223372036854775807>
>> pointer_to_this <pointer_type 0x7ffff65e0498>>
>>
>> arg 0 <nop_expr 0x7ffff65cbb60
>> type <pointer_type 0x7ffff65e0498 type <integer_type 0x7ffff64a77e0
>> long int>
>> public static unsigned DI size <integer_cst 0x7ffff64a3ca8 64>
>> unit size <integer_cst 0x7ffff64a3cc0 8>
>> align 64 symtab 0 alias set -1 canonical type 0x7ffff65e0498>
>>
>> arg 0 <var_decl 0x7ffff65e32d0 addr.2 type <pointer_type
>> 0x7ffff64c2150>
>> used unsigned ignored DI file stdarg-1.c line 4 col 1 size
>> <integer_cst 0x7ffff64a3ca8 64> unit size <integer_cst 0x7ffff64a3cc0 8>
>> align 64 context <function_decl 0x7ffff65c21b0 f2>>>
>> arg 1 <integer_cst 0x7ffff65e64e0 type <pointer_type 0x7ffff65e0498>
>> constant 0>>
>> ...
>>
>> During 'gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either)' we ICE
>> here:
>> ...
>> 8886 gcc_assert ((*gimple_test_f) (*expr_p));
>> ...
>>
>> At this point expr is:
>> ...
>> (gdb) call debug_generic_expr (*expr_p)
>> *addr.2
>> ...
>>
>> In more detail:
>> ...
>> (gdb) call debug_tree (*expr_p)
>> <mem_ref 0x7ffff65ea1e0
>> type <void_type 0x7ffff64c2000 void VOID
>> align 8 symtab 0 alias set -1 canonical type 0x7ffff64c2000
>> pointer_to_this <pointer_type 0x7ffff64c2150>>
>>
>> arg 0 <var_decl 0x7ffff65e32d0 addr.2
>> type <pointer_type 0x7ffff64c2150 type <void_type 0x7ffff64c2000
>> void>
>> sizes-gimplified public unsigned DI
>> size <integer_cst 0x7ffff64a3ca8 constant 64>
>> unit size <integer_cst 0x7ffff64a3cc0 constant 8>
>> align 64 symtab 0 alias set -1 canonical type 0x7ffff64c2150
>> pointer_to_this <pointer_type 0x7ffff64c9bd0>>
>> used unsigned ignored DI file stdarg-1.c line 4 col 1 size
>> <integer_cst 0x7ffff64a3ca8 64> unit size <integer_cst 0x7ffff64a3cc0 8>
>> align 64 context <function_decl 0x7ffff65c21b0 f2>>
>> arg 1 <integer_cst 0x7ffff64c10c0 type <pointer_type 0x7ffff64c2150>
>> constant 0>>
>> ...
>>
>> The memref is now VOID type. And that's not a gimple_val:
>> ...
>> (gdb) p is_gimple_val (*expr_p)
>> $1 = false
>> ...
>
> On which target? I can't seem to reproduce on x86_64 or i?86. I also
> can't see how this
> could happen.
>
Reproduced using attached patch on top of r222537, for target x86_64, with and
without -m32.
Thanks,
- Tom
> Richard.
>
>> Attached patch uses is_gimple_lvalue, but in expand_ifn_va_arg_1.
>>
>> I'll bootstrap and reg-test on x86_64 and commit, unless further comments of
>> course.
>>
>> Thanks,
>> - Tom
[-- Attachment #2: 0001-Reproduce-ICE-in-gimplify_expr-gcc_assert-gimple_tes.patch --]
[-- Type: text/x-patch, Size: 875 bytes --]
Reproduce ICE in 'gimplify_expr:
gcc_assert ((*gimple_test_f) (*expr_p))'
diff --git a/gcc/testsuite/gcc.dg/reproduce.c b/gcc/testsuite/gcc.dg/reproduce.c
new file mode 100644
index 0000000..c0c8e0b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/reproduce.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#include <stdarg.h>
+
+void
+f2 (int i, ...)
+{
+ va_list ap;
+ va_start (ap, i);
+ va_arg (ap, long);
+ va_end (ap);
+}
diff --git a/gcc/tree-stdarg.c b/gcc/tree-stdarg.c
index 1356374..829cbc1 100644
--- a/gcc/tree-stdarg.c
+++ b/gcc/tree-stdarg.c
@@ -1079,7 +1079,7 @@ expand_ifn_va_arg_1 (function *fun)
gimplify_assign (lhs, expr, &pre);
}
else
- gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
+ gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either);
pop_gimplify_context (NULL);
--
1.9.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][PR65818][bootstrap,hppa] Return side-effect free result in gimplify_va_arg_internal
2015-04-28 21:59 ` Tom de Vries
@ 2015-04-29 9:53 ` Richard Biener
0 siblings, 0 replies; 8+ messages in thread
From: Richard Biener @ 2015-04-29 9:53 UTC (permalink / raw)
To: Tom de Vries; +Cc: GCC Patches, John David Anglin
On Tue, Apr 28, 2015 at 10:29 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 28-04-15 12:34, Richard Biener wrote:
>>
>> On Mon, Apr 27, 2015 at 5:04 PM, Tom de Vries <Tom_deVries@mentor.com>
>> wrote:
>>>
>>> On 27-04-15 15:40, Richard Biener wrote:
>>>>
>>>>
>>>> On Mon, Apr 27, 2015 at 3:06 PM, Tom de Vries <Tom_deVries@mentor.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 27-04-15 10:17, Richard Biener wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> This patch fixes that by gimplifying the address expression of the
>>>>>>> mem-ref
>>>>>>>>
>>>>>>>>
>>>>>>>> returned by the target hook (borrowing code from gimplify_expr, case
>>>>>>>> MEM_REF).
>>>>>>>>
>>>>>>>> Bootstrapped and reg-tested on x86_64.
>>>>>>>>
>>>>>>>> Bootstrapped and reg-tested on hppa2.0w-hp-hpux11.11.
>>>>>>>>
>>>>>>>> OK for trunk?
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hmm, that assert looks suspicious...
>>>>>>
>>>>>> Can't you simply always do
>>>>>>
>>>>>> gimplify_expr (expr, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> It's a bit counter-intuitive for me, using is_gimple_lvalue for
>>>>> something
>>>>> (the result of va_arg) we use as rvalue.
>>>>
>>>>
>>>>
>>>> Yeah, choosing that was done because you could assert it's a MEM_REF
>>>> and tree-stdarg eventually builds a WITH_SIZE_EXPR around it.
>>>>
>>>> It would possibly be easier to simply "inline" gimplify_va_arg_internal
>>>> at its use and only gimplify the assignment. Though in that case the
>>>> original code probably worked - just in the lhs == NULL case it didn't,
>>>> which hints at a better place for the fix - in expand_ifn_va_arg_1 do
>>>>
>>>> if (lhs != NULL_TREE)
>>>> {
>>>> ...
>>>> }
>>>> else
>>>> gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either);
>>>>
>>>> So ... if you can re-try that one it's pre-approved.
>>>>
>>>
>>> Using that proposed patch, we run into the following problem.
>>>
>>> Consider this testcase with ignored-result-va_arg:
>>> ...
>>> #include <stdarg.h>
>>>
>>> void
>>> f2 (int i, ...)
>>> {
>>> va_list ap;
>>> va_start (ap, i);
>>> va_arg (ap, long);
>>> va_end (ap);
>>> }
>>> ...
>>>
>>> after gimplify_va_arg_internal we have:
>>> ...
>>> (gdb) call debug_generic_expr (expr)
>>> *(long int * {ref-all}) addr.2
>>> ...
>>>
>>> In a bit more detail:
>>> ...
>>> (gdb) call debug_tree (expr)
>>> <mem_ref 0x7ffff65ea1b8
>>> type <integer_type 0x7ffff64a77e0 long int DI
>>> size <integer_cst 0x7ffff64a3ca8 constant 64>
>>> unit size <integer_cst 0x7ffff64a3cc0 constant 8>
>>> align 64 symtab 0 alias set -1 canonical type 0x7ffff64a77e0
>>> precision 64 min <integer_cst 0x7ffff64a3f30 -9223372036854775808> max
>>> <integer_cst 0x7ffff64a3f48 9223372036854775807>
>>> pointer_to_this <pointer_type 0x7ffff65e0498>>
>>>
>>> arg 0 <nop_expr 0x7ffff65cbb60
>>> type <pointer_type 0x7ffff65e0498 type <integer_type
>>> 0x7ffff64a77e0
>>> long int>
>>> public static unsigned DI size <integer_cst 0x7ffff64a3ca8
>>> 64>
>>> unit size <integer_cst 0x7ffff64a3cc0 8>
>>> align 64 symtab 0 alias set -1 canonical type
>>> 0x7ffff65e0498>
>>>
>>> arg 0 <var_decl 0x7ffff65e32d0 addr.2 type <pointer_type
>>> 0x7ffff64c2150>
>>> used unsigned ignored DI file stdarg-1.c line 4 col 1 size
>>> <integer_cst 0x7ffff64a3ca8 64> unit size <integer_cst 0x7ffff64a3cc0 8>
>>> align 64 context <function_decl 0x7ffff65c21b0 f2>>>
>>> arg 1 <integer_cst 0x7ffff65e64e0 type <pointer_type 0x7ffff65e0498>
>>> constant 0>>
>>> ...
>>>
>>> During 'gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either)' we
>>> ICE
>>> here:
>>> ...
>>> 8886 gcc_assert ((*gimple_test_f) (*expr_p));
>>> ...
>>>
>>> At this point expr is:
>>> ...
>>> (gdb) call debug_generic_expr (*expr_p)
>>> *addr.2
>>> ...
>>>
>>> In more detail:
>>> ...
>>> (gdb) call debug_tree (*expr_p)
>>> <mem_ref 0x7ffff65ea1e0
>>> type <void_type 0x7ffff64c2000 void VOID
>>> align 8 symtab 0 alias set -1 canonical type 0x7ffff64c2000
>>> pointer_to_this <pointer_type 0x7ffff64c2150>>
>>>
>>> arg 0 <var_decl 0x7ffff65e32d0 addr.2
>>> type <pointer_type 0x7ffff64c2150 type <void_type 0x7ffff64c2000
>>> void>
>>> sizes-gimplified public unsigned DI
>>> size <integer_cst 0x7ffff64a3ca8 constant 64>
>>> unit size <integer_cst 0x7ffff64a3cc0 constant 8>
>>> align 64 symtab 0 alias set -1 canonical type 0x7ffff64c2150
>>> pointer_to_this <pointer_type 0x7ffff64c9bd0>>
>>> used unsigned ignored DI file stdarg-1.c line 4 col 1 size
>>> <integer_cst 0x7ffff64a3ca8 64> unit size <integer_cst 0x7ffff64a3cc0 8>
>>> align 64 context <function_decl 0x7ffff65c21b0 f2>>
>>> arg 1 <integer_cst 0x7ffff64c10c0 type <pointer_type 0x7ffff64c2150>
>>> constant 0>>
>>> ...
>>>
>>> The memref is now VOID type. And that's not a gimple_val:
>>> ...
>>> (gdb) p is_gimple_val (*expr_p)
>>> $1 = false
>>> ...
>>
>>
>> On which target? I can't seem to reproduce on x86_64 or i?86. I also
>> can't see how this
>> could happen.
>>
>
> Reproduced using attached patch on top of r222537, for target x86_64, with
> and without -m32.
Ok, I see now. The gimplifier seems to be confused with fb_lvalue.
all is_gimple_addressable ()s are already is_gimple_lvalue so
Index: gcc/gimplify.c
===================================================================
--- gcc/gimplify.c (revision 222537)
+++ gcc/gimplify.c (working copy)
@@ -8844,15 +8844,9 @@ gimplify_expr (tree *expr_p, gimple_seq
rvalue. */
if ((fallback & fb_lvalue)
&& gimple_seq_empty_p (internal_post)
- && is_gimple_addressable (*expr_p))
- {
- /* An lvalue will do. Take the address of the expression, store it
- in a temporary, and replace the expression with an INDIRECT_REF of
- that temporary. */
- tmp = build_fold_addr_expr_loc (input_location, *expr_p);
- gimplify_expr (&tmp, pre_p, post_p, is_gimple_reg, fb_rvalue);
- *expr_p = build_simple_mem_ref (tmp);
- }
+ && is_gimple_lvalue (*expr_p))
+ /* An lvalue will do and we already have one. */
+ ;
else if ((fallback & fb_rvalue) && is_gimple_reg_rhs_or_call (*expr_p))
{
/* An rvalue will do. Assign the gimplified expression into a
should be enough for that case. OTOH is_gimple_val will not be true
after this, so we'd still ICE later. Which means fb_lvalue doesn't make
sense for is_gimple_val.
So your patch looks ok.
Richard.
> Thanks,
> - Tom
>
>
>> Richard.
>>
>>> Attached patch uses is_gimple_lvalue, but in expand_ifn_va_arg_1.
>>>
>>> I'll bootstrap and reg-test on x86_64 and commit, unless further comments
>>> of
>>> course.
>>>
>>> Thanks,
>>> - Tom
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-04-29 9:30 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-25 12:35 [PATCH][PR65818][bootstrap,hppa] Return side-effect free result in gimplify_va_arg_internal Tom de Vries
2015-04-27 8:17 ` Richard Biener
2015-04-27 13:06 ` Tom de Vries
2015-04-27 13:40 ` Richard Biener
2015-04-27 15:04 ` Tom de Vries
2015-04-28 10:49 ` Richard Biener
2015-04-28 21:59 ` Tom de Vries
2015-04-29 9:53 ` Richard Biener
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).