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

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

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

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.

[-- Attachment #2: p.diff.txt --]
[-- Type: text/plain, Size: 453 bytes --]

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;
 

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

[Richard Henderson]

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~

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

[Uros Bizjak]

On Mon, Aug 2, 2010 at 6:59 PM, Richard Henderson <rth@redhat.com> wrote:

>> 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.

Thanks. I have committed the patch with a comment:

Index: alpha.c
===================================================================
--- alpha.c	(revision 162825)
+++ alpha.c	(working copy)
@@ -5950,6 +5950,10 @@
 		    integer_type_node);
   DECL_FIELD_CONTEXT (ofs) = record;
   DECL_CHAIN (ofs) = space;
+  /* ??? This is a hack, __offset is marked volatile to prevent
+     DCE that confuses stdarg optimization and results in
+     gcc.c-torture/execute/stdarg-1.c failure.  See PR 41089.  */
+  TREE_THIS_VOLATILE (ofs) = 1;

   base = build_decl (BUILTINS_LOCATION,
 		     FIELD_DECL, get_identifier ("__base"),

Uros.

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

[Richard Guenther]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2229 bytes --]

On Mon, 2 Aug 2010, Uros Bizjak wrote:

> On Mon, Aug 2, 2010 at 6:59 PM, Richard Henderson <rth@redhat.com> wrote:
> 
> >> 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.
> 
> Thanks. I have committed the patch with a comment:
> 
> Index: alpha.c
> ===================================================================
> --- alpha.c	(revision 162825)
> +++ alpha.c	(working copy)
> @@ -5950,6 +5950,10 @@
>  		    integer_type_node);
>    DECL_FIELD_CONTEXT (ofs) = record;
>    DECL_CHAIN (ofs) = space;
> +  /* ??? This is a hack, __offset is marked volatile to prevent
> +     DCE that confuses stdarg optimization and results in
> +     gcc.c-torture/execute/stdarg-1.c failure.  See PR 41089.  */
> +  TREE_THIS_VOLATILE (ofs) = 1;
> 
>    base = build_decl (BUILTINS_LOCATION,
>  		     FIELD_DECL, get_identifier ("__base"),

I wonder if we can't use points-to information instead of trying
to match the lowered accesses.

Richard.