[Bug lto/50494] gcc.dg/vect/vect-reduc-2char.c fails spuriously on ppc with -flto

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50494

--- Comment #33 from rguenther at suse dot de <rguenther at suse dot de> 2013-03-05 15:48:03 UTC ---
On Tue, 5 Mar 2013, ebotcazou at gcc dot gnu.org wrote:

> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50494
> 
> --- Comment #32 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-05 15:32:15 UTC ---
> > But in all places I found we check TREE_ASM_WRITTEN on DECL_INITIAL
> > of the SYMBOL_REF_DECL ...
> 
> Nope, maybe_output_constant_def_contents has:
> 
>   rtx symbol = XEXP (desc->rtl, 0);
>   tree exp = desc->value;
> 
>   if (flag_syntax_only)
>     return;
> 
>   if (TREE_ASM_WRITTEN (exp))
>     /* Already output; don't do it again.  */
>     return;
> 
> so the DECL_INITIAL of the SYMBOL_REF_DECL must be desc->value.

Ah, ok ... too many smart TREE_ASM_WRITTEN bits around ...

> > So it must be pure luck that we survived LTO bootstrap before my
> > patch (as far as I understand things).  Before my patch we created
> > yet another decl for the constant pool entry.  With my patch
> > we will have one less (we still have the decls from the constant
> > pool entries that end up being shared in the LTRANS unit).
> 
> We use LTO on heavy Ada applications with an unmodified 4.7 compiler (modulo a
> patch for -g) so I don't think that luck has anything to do here.

Fine, so the above might be the only issue.

> > So in the end I can agree to that my patch doesn't really fix
> > the original issue fully.  So we can as well revert it and
> > instead avoid messing with alignment of the constant pool entries.
> 
> That would be my preference.

Which of course pessimizes code generation and probably causes
some testsuite fallout for ppc (the original reported spurious
fails).

> > Hmm, maybe.  Then, why do we copy the constant in the first place ...
> > 
> > Thus,
> > 
> > Index: varasm.c
> > ===================================================================
> > --- varasm.c    (revision 196462)
> > +++ varasm.c    (working copy)
> > @@ -3087,7 +3087,7 @@ build_constant_desc (tree exp, tree decl
> >    int labelno;
> > 
> >    desc = ggc_alloc_constant_descriptor_tree ();
> > -  desc->value = copy_constant (exp);
> > +  desc->value = exp;
> > 
> >    /* Propagate marked-ness to copied constant.  */
> >    if (flag_mudflap && mf_marked_p (exp))
> > 
> > should be an "equivalent" fix.
> 
> This call to copy_constant has been there for ages though. so a little bit of
> archeology would probably be in order before removing it.

;)  Did that, it's there since forever - well, I traced it back to
the point we only deferred string constants:

 37459      jakub                 p = (struct deferred_string *)
 37459      jakub                     xmalloc (sizeof (struct 
deferred_string));
 37459      jakub 
 37459      jakub                 p->exp = copy_constant (exp);
 37459      jakub                 p->label = desc->label;

I'm LTO bootstrapping

  desc->value = decl ? exp : copy_constant (exp);

and doing a regular bootstrap with the copy_constant completely removed
at the moment.  Just curious ...

> In the meantime, I've successfully bootstrapped my patchlet so we can also go
> for it.

I'm fine with that.

As I concluded that the original fix didn't fix the alignment issue
(well, not for all possible cases at least) reverting the original
fix works for me as well.

I'm working on a patch to avoid re-aligning constant pool entries.