This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PING] tighten gimple addressability checks


Richard Guenther wrote:

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

> But never for non byte-aligned storage, right?

 Right.

> So, I presume the gimplifier currently takes the address of non-byte-aligned
> objects?

 Yes.

> If so, the gimplifier should be fixed (either via adjusting the
> is_gimple_addressable predicate or fixing the specific places where
> it happens).

 Right. This is exactly the intention with this patch, as we did
 a couple of times for other situations in the past.

> I don't think addressability should be constrained on
> strict-alignment targets.
 
 Depends on what we mean by "addressability" :) More below.

> >  It used to happen for integral modes as well and that was causing
> >  trouble [...]  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?

 No : what is already fixed is the attempt to memcmp for a comparison of
 scalar mode aggregates. This was fixed long ago. I mentioned this case
 as an example of past issues similar to the one we are trying to address
 now.

 What we are trying to address now is a similar problem (the gimplifier
 taking a the address of a non-byte-aligned entity) in a different situation:
 while processing a MODIFY_EXPR.
 
 There are two parts of why it is doing that. The current code reads

 << gimplify_modify_expr
    ...
  /* If we've got a variable sized assignment between two lvalues (i.e. does
     not involve a call), then we can make things a bit more straightforward
     by converting the assignment to memcpy or memset.  */
  if (TREE_CODE (*from_p) == WITH_SIZE_EXPR)
    {
      ...
      if (TREE_CODE (from) == CONSTRUCTOR)
        return gimplify_modify_expr_to_memset (expr_p, size, want_value);
      if (is_gimple_addressable (from))
        {
          *from_p = from;
          return gimplify_modify_expr_to_memcpy (expr_p, size, want_value);
        }
    }
 >>

 First, there is no check that the destination is "addressable", and second
 is what we have been discussing: what such a check should do.

 One of the hunks in the patch addresses the first aspect (check whether
 'to' is addressable here).

 The other hunks deal with the second aspect, and this is a bit
 involved because of the introduction of the 'block operation' notion. 
 If that notion turns out to be useless, and it might well be, the
 patch will be much simpler. Again, more on that below.

> I believe (but didn't verify) that the C frontend does not set DECL_BIT_FIELD
> on STRICT_ALIGNMENT targets on the FIELD_DECL for b in
> 
> struct __attribute__((packed) {
>   char a;
>   int b;
> };

 Indeed it doesn't. The Ada front-end does, I think to notify that bitfield
 ops are needed to access '.b'.

> whether it is ok to take the address depends only on if the address
> is representable (that is, no bit-addressing).

 Interesting. I actually wondered a lot about that and the whole
 'block operation' thing was an assumption that 'addressable' was
 a more convoluted notion.

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

 I need to give it a try. The patch would be a lot simpler if this is
 indeed not needed.
 
> >  The suggested is_gimple_addressable_1 predicate is intended as a
> >  conservative approximation of this property. [...]
 
> 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? 

 There was no attempt to allow that. The need didn't show up so far and
 looking for it would be more expensive.

> If not, we can bail out from the recursion once we reach a non-zero
> bit offset.
 
 I see, sounds good to me. The initial suggestion (DECL_BIT_FIELD &&
 !BLKmode) was close, and I don't see why a straight bit offset check
 wouldn't work.

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

 Precisely.

 I think we need to experiment variants at this point. I don't quite
 remember how we are processing the assignment at fault later on (if
 it's not handled by a memcpy) and need to double check that as well.

 Many thanks again for your very constructive feedback,

 Olivier




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]