diff mbox

[alpha] : Fix PR target/41089, stdarg pass produces wrong code

Message ID AANLkTingvVPJPfnaheLHWgpyGPuGbxGLXERsP+j0zjx5@mail.gmail.com
State New
Headers show

Commit Message

Uros Bizjak Aug. 1, 2010, 6:44 p.m. UTC
Hello!

As discussed in the PR, stdarg pass depends on number of assignments
to ap.__offset location for correct operation. However, recent FRE/DCE
enhancements remove one assignment as a dead code, causing the test
failure:

FAIL: gcc.c-torture/execute/stdarg-1.c execution,  -O3 -fomit-frame-pointer
FAIL: gcc.c-torture/execute/stdarg-1.c execution,  -O3 -g

Attached patch marks __offset as volatile (suggested by Richi in
comment #39), preventing optimizations that could otherwise confuse
stdarg pass.

2010-08-01  Uros Bizjak  <ubizjak@gmail.com>

	PR target/41089
	* config/alpha/alpha.c (alpha_build_builtin_va_list): Mark __offset
	as volatile.

Patch was bootstrapped and regression tested on
alphaev68-pc-linux-gnu, where it fixes the failure.

OK for mainline/4.5?

Uros.

Comments

Richard Henderson Aug. 2, 2010, 4:59 p.m. UTC | #1
On 08/01/2010 11:44 AM, Uros Bizjak wrote:
> Hello!
> 
> As discussed in the PR, stdarg pass depends on number of assignments
> to ap.__offset location for correct operation. However, recent FRE/DCE
> enhancements remove one assignment as a dead code, causing the test
> failure:
> 
> FAIL: gcc.c-torture/execute/stdarg-1.c execution,  -O3 -fomit-frame-pointer
> FAIL: gcc.c-torture/execute/stdarg-1.c execution,  -O3 -g
> 
> Attached patch marks __offset as volatile (suggested by Richi in
> comment #39), preventing optimizations that could otherwise confuse
> stdarg pass.
> 
> 2010-08-01  Uros Bizjak  <ubizjak@gmail.com>
> 
> 	PR target/41089
> 	* config/alpha/alpha.c (alpha_build_builtin_va_list): Mark __offset
> 	as volatile.
> 
> Patch was bootstrapped and regression tested on
> alphaev68-pc-linux-gnu, where it fixes the failure.

Ug.  That's awful.  It means that when you really do have dead code
it won't get deleted, and thus the stdarg pass won't be able to 
eliminate the stores that could have been prevented.

I'm tempted to say just remove the entire optimization, but what's
left after the volatile may be slightly better than no optimization
at all -- i.e. we can determine that there were never any va_arg
requests for double at all.

So... patch ok.

That said, I begin to think that a simpler solution in the long run
might be to avoid expanding va_arg before pass_stdarg at all.  Then
we don't need to do any of this kind of queer code scanning or hacks
in sra to prevent structure decomp.  We'd simply scan for whether
the data escapes, and with the help of a callback record the set of
types read.  After pass_stdarg, va_arg would be decomposed and we 
could perform all the usual cleanups.


r~
diff mbox

Patch

Index: config/alpha/alpha.c
===================================================================
--- config/alpha/alpha.c	(revision 162794)
+++ config/alpha/alpha.c	(working copy)
@@ -5948,6 +5948,7 @@  alpha_build_builtin_va_list (void)
   ofs = build_decl (BUILTINS_LOCATION,
 		    FIELD_DECL, get_identifier ("__offset"),
 		    integer_type_node);
+  TREE_THIS_VOLATILE (ofs) = 1;
   DECL_FIELD_CONTEXT (ofs) = record;
   DECL_CHAIN (ofs) = space;