[PING] tighten gimple addressability checks

[Richard Guenther]

On Dec 10, 2007 6:13 PM, Olivier Hainque <hainque@adacore.com> wrote:
>
> [Thanks for your feedback]
>
> Richard Guenther wrote:
> > So, why are there ADDR_EXPRs of non-addressable variables at all?
>
>  Because the gimplifier takes them for misc purposes and that's what the patch
>  is trying to address.

Ok.

>  The initial case (with the patch submission) was gimplify_modify_expr
>  turning an assignment into a call to memcpy or memset without looking too much
>  (and part of the patch changes that).
>
>  In the other case I posted (the one with a comparison), the comparison is turned
>  into a memcmp from
>
>       gimplify_expr (tree *expr_p, tree *pre_p, tree *post_p,
>       ...
>             case tcc_comparison:
>               /* Handle comparison of objects of non scalar mode aggregates
>                  with a call to memcmp.  [...]
>
> > I think in the end we want the frontend to sort out what is addressable and what not
> > (and we have TREE_ADDRESSABLE for that).
>
> > Do you say, that an outer component a.b is not addressable, but a.b.c is?
>
>  Hummm, not really.
>
>  Suppose you have an 'int' at byte position 1 in a struct. What I was saying (or
>  trying to say) is that
>
>    it is not generally safe to take and use the address of that 'int', because the
>    address is likely not aligned in accordance with the type,

That does not defeat addressability (if at least byte-alignment (BITS_PER_UNIT)
is guaranteed) - IMHO.

>  but
>
>    it's ok in specific circumstances where this sort of misalignment doesn't matter,

But never for non byte-aligned storage, right?

>  and
>
>    using the address to perform a block oriented operation (like memcpy or memcmp)
>    is one such specific circumstance.

So, I presume the gimplifier currently takes the address of non-byte-aligned
objects?  If so, the gimplifier should be fixed (either via adjusting the
is_gimple_addressable predicate or fixing the specific places where it happens).

I don't think addressability should be constrained on strict-alignment targets.

> > Is the usage of memcmp appreciated here?  Is it even correct in general?!
>
>  We haven't introduced it so I don't know for sure. It's now only issued for
>  BLKmode and that looks reasonable because we know the operands are at least byte
>  aligned.
>
>  It used to happen for integral modes as well and that was causing trouble (was
>  inefficient at best and blowing up for bit-aligned operands).  This is one of the
>  former changes we made.

So this is already fixed.  Is the patch as is then just addressing the
performance
problem?

> > Maybe the problem is "DECL_BIT_FIELD set on STRICT_ALIGNMENT targets"
> >  - I don't think the C frontend does this for packed structures.
>
>  I'm not sure what you mean here.

I believe (but didn't verify) that the C frontend does not set DECL_BIT_FIELD
on the FIELD_DECL for b in

struct __attribute__((packed) {
  char a;
  int b;
};

on STRICT_ALIGNMENT targets.  And I wouldn't see why it should.  Neither
should any of a or b be not TREE_ADDRESSABLE.

> > we might have to recurse here and look for DECL_FIELD_BIT_OFFSET
> > (or rather, TREE_ADDRESSABLE on the FIELD_DECLs).
>
>  Humm, maybe. Here's how our attempt came along (roughly):
>
>  Whether it's ok to take the address depends on how this address is
>  going to be used. In the general case we don't know, but we do know
>  when we are doing so from the gimplifier and can provide a useful hint
>  about it.

I think you are wrong here - whether it is ok to take the address depends only
on if the address is representable (that is, no bit-addressing).

>  This is what BLOCK_ADDRESS_P is about in the patch: to state that this ADDR_EXPR
>  is formed to be used as an argument for of a block operation like memcpy, hence
>  that byte alignment of the operand is good enough.

Expansion deals with non-aligned addresses and de-references on
STRICT_ALIGNMENT targets, so this should be not needed.

>  The suggested is_gimple_addressable_1 predicate is intended as a conservative
>  approximation of this property. It bails out (claims not addressable) fairly
>  easily, a compromise to be precise enough in practice, while remaining simple
>  and stopping the recursion more rapidly.

This is where I asked about the a.b vs. a.b.c thing, if a.b is at
bit-offset 1, do
we allow (and want) a.b.c addressable if a.b.c happens to be at bit-offset 0
again?  If not, we can bail out from the recursion once we reach a non-zero
bit offset.

>  I first thought of using get_inner_reference, and then that DECL_BIT_FIELD is a
>  fairly precise hint already.

Yes, though get_inner_reference is quite expensive.

> > >  we get down to expand_expr_real_1 with 'exp' like
> ...
> > >  and it works just fine.
> >
> > I see.  I guess the fact that we cannot emulate this in C means that
> > occasional bugs in tree optimizers will sneak in.  Does the Ada testsuite
> > provide good coverage of those corner-cases?
>
>  It's certainly not exhaustive. We're adding the cases as we get the patches in.

Thanks,
Richard.