diff mbox

[PR65818,bootstrap,hppa] Return side-effect free result in gimplify_va_arg_internal

Message ID 553E3451.5010300@mentor.com
State New
Headers show

Commit Message

Tom de Vries April 27, 2015, 1:06 p.m. UTC 
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

Comments

Richard Biener April 27, 2015, 1:40 p.m. UTC | #1 
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
>
diff mbox

Patch

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