Re: [patch] PR32901: handle assignment...(and PR34465 and PR34448)

[Jakub Jelinek <jakub at redhat dot com>]

On Wed, Dec 19, 2007 at 06:27:34AM -0500, Diego Novillo wrote:
> On 12/14/07 16:25, Aldy Hernandez wrote:
> 
> >Also... in a subsequent patch we should really look into merging
> >consecutive initializations of aggregate fields-- sometime before
> >expand.  Jakub has suggested TER.  I volunteer to do the work once I'm
> >done with this.
> 
> Any reason why you couldn't do this directly in the gimplifier?  Instead 
> of emitting the separate field assignments, just emit a single 
> CONSTRUCTOR assignment.  You already have the DECL_INITIAL constructor 
> after all.  I haven't thought it through, so the suggestion may be nonsense.

ATM non-empty CONSTRUCTOR on RHS (unless it is a vector CONSTRUCTOR) is
considered invalid GIMPLE.  I guess there are good reasons for that,
otherwise many optimizations would need to be taught how to pick individual
elements from the constructor etc.  Furthermore, I guess we want to optimize
even cases where the CONSTRUCTOR wasn't there in user's code:

typedef struct __attribute__((aligned (4))) { unsigned char a, b, c, d; } T;
typedef struct { unsigned int a : 5, b : 5, c : 5, d : 5, e : 5, f : 5; } S;

static T u, v;
static S s, t;

void
foo (void)
{
  u = (T) { 1, 2, 3, 4};
  v.a = 1;
  v.b = 2;
  v.c = 3;
  v.d = 4;
}
 
void
bar (void)
{
  s = (S) { 1, 2, 3, 4, 5, 6 };
  t.a = 1;
  t.b = 2;
  t.c = 3;
  t.d = 4;
  t.e = 5;
  t.f = 6;
}

IMHO this should be optimized the same for u and v stores, particularly
just store 0x01020304 (resp. 0x04030201 depending on endianity) to u
and v, with the alias set of the whole T for the stores.  The bar assembly
is complete disaster.  For bar 3.4 the store to s is just:
        movl    $206703681, s(%rip)     #, s
while trunk needs 20 times more instructions to do the same.
The reason why I mentioned TER is that such an optimization would create
invalid GIMPLE (so should be done as late as possible), but to do it
properly we need SSA form, so we can walk the VDEFs to collect the
individual constructor elements.  When expand sees the whole constructor,
it can make better decisions with it, compared to what it can do when it
sees just individual stores.

For bitfields, even if there are just a few consecutive stores (or same
bitwise ops, e.g.
t.a = 1;
t.b = 2;
t.c = 3;
or
t.a |= 4;
t.b |= 4;
t.c |= 4;
from above example) it might be beneficial to aggregate them together, but
not sure how to represent that at the tree level.  But that can be done
separately.

> >@@ -3119,11 +3119,18 @@ gimplify_init_ctor_eval (tree object, VE
> > 
> >    Note that we still need to clear any elements that don't have explicit
> >    initializers, so if not all elements are initialized we keep the
> >-   original MODIFY_EXPR, we just remove all of the constructor elements.  
> >*/
> >+   original MODIFY_EXPR, we just remove all of the constructor elements.
> >+
> >+   If NOTIFY_TEMP_CREATION is true, do not gimplify, just return
> >+   GS_ERROR if we would have to create a temporary when gimplifying
> >+   this constructor.  Otherwise, return GS_OK.
> >+
> >+   If NOTIFY_TEMP_CREATION is false, just do the gimplification.  */
> 
> Hmm, I don't much like this part.  This function is supposed to just 
> gimplify the constructor.  The tests for NOTIFY_TEMP_CREATION are simple 
> enough to be hoisted into a single predicate for 
> gimplify_modify_expr_rhs to use.
> 
> Otherwise, this one function grows in complexity and it already is a bit 
> convoluted.

Could just a name change of the function help?  The thing is that
both the predicate and the actual gimplification of the CONSTRUCTOR needs
to compute a lot of stuff (categorize_ctor_elements itself sets 4 different
things, then num_type_elements, further cleared adjustments, size,
alignment), so if we have an predicate and gimplify_init_constructor as
separate functions, we'd either need to duplicate a lot of stuff, or e.g.
have some helper function which precomputes a lot of stuff into some temp
structure, from which the predicate and gimplify_init_constructor then use it.

	Jakub