Re: [PATCH] Fix constant array ARRAY_REF[constant index] optimization (PR middle-end/28755)

[Jakub Jelinek <jakub at redhat dot com>]

On Tue, Aug 22, 2006 at 03:29:10PM +0100, Richard Sandiford wrote:
> Jakub Jelinek <jakub@redhat.com> writes:
> > As shown by the attached testcase, the constant array ARRAY_REF
> > with constant index optimization is in some cases really big pessimization.
> > If expand_expr decides to put a CONSTRUCTOR into memory, then we just waste
> > rodata memory, while we in that case really should just use the original
> > array entry.
> > The following patches (attached 4.1 and 4.2 versions) check for this and
> > if it determines it is not a win, it falls back to using the actual
> > original array reference.  On 4.1 that works without a problem,
> > unfortunately in 4.2 it seems -fsection-anchors kills the garbage collection
> > of unused deferred constants.
> 
> Can you give more details?

static rtx
expand_expr_constant (tree exp, int defer, enum expand_modifier modifier)
{
  rtx mem;

  mem = output_constant_def (exp, defer);
  if (modifier != EXPAND_INITIALIZER)
    mem = use_anchored_address (mem);
  return mem;
}

Several places in expr.c call this routine with defer = 1, which means that
the constant shouldn't be output right away, only the symbol created and
it will be output if needed (i.e. if referenced at the end of RTL
optimizations).  At least that's how it used to work.
See also maybe_output_constant_def_contents and
output_constant_pool/mark_constant_pool.
But, with -fsection-anchors, use_anchored_address will "use" the constant
right away, immediately allocates space for it in the anchor data block
and returns the anchor symbol plus computed offset.  For defer == 0
that's fine, but for defer != 0 that's a bad idea.  Either we shouldn't
call use_anchored_address if defer != 0, or we should, but return from
it some new RTL expression which would mean anchor data block offset
of SYMBOL_REF xyz and let that RTL live at least through the first
half of RTL optimizations, so that it can be optimized away if not needed
and only if still needed be expanded into some actual CONST_INT.

> 
> > I have worked around it in the patch, but think it should be
> > eventually fixed in the -fsection-anchors handling instead and then
> > this hack can be removed.
> >
> > +
> > +			  /* For CONSTRUCTOR this optimization is not always
> > +			     a win - if expand_expr creates a temporary
> > +			     constant, we just waste unnecessarily rodata
> > +			     space.
> > +			     The section_anchors hack is ugly, but ATM
> > +			     flag_section_anchors kills the ability to
> > +			     garbage collect unused deferred constants
> > +			     and we need that here.  */
> > +			  save_section_anchors = flag_section_anchors;
> > +			  flag_section_anchors = 0;
> > +			  temp = expand_expr (value, target, tmode, modifier);
> > +			  flag_section_anchors = save_section_anchors;
> > +			  if (temp == target
> > +			      || (temp && GET_CODE (temp) != MEM))
> > +			    {
> > +			      if (modifier != EXPAND_INITIALIZER
> > +				  && temp != target
> > +				  && temp)
> > +				temp = use_anchored_address (temp);
> > +			      return temp;
> > +			    }
> 
> Ugh.  I don't think this is accetable personally, although it's
> obviously not my call.

I agree it is very ugly, but not being able to garbage collect
deferred constants because of -fanchor-sections which it is trying
to hack around is far worse bug.
You can apply the GCC 4.1 patch I posted against trunk tree
and try the testcase under gdb to see what it will do.

	Jakub