[patch] Make memory copy functions scalar storage order barriers

[Richard Biener]

On Tue, Jun 2, 2020 at 7:11 PM Eric Botcazou <botcazou@adacore.com> wrote:
>
> [Starting with tne VN issue]
>
> > For the VN case the issue is interpreting a read via memcpy (non
> > reverse-storage) as a reverse-storage one when matching up with a hashtable
> > entry from a regular reverse-storage order store, correct?
>
> It's a read from a scalar (hence native order) combined with a store to an
> aggregate type with reverse order:
>
>    u = 305419896;
>   __builtin_memcpy (&tempb, &u, 4);
>   _1 = tempb.val;
>
> > But likewise whether the pointer destination had the attribute doesn't
> > correctly qualify this - instead I think we are already properly matching up
> > the reverse storage property?
>
> No, we do not do direct matching for the storage order because you may not
> access the same memory location with different scalar storage orders, it's the
> fundamental invariant, so it's redundant with e.g. the offset for matching.
>
> You need to byte swap if you want to propagate 305419896 to _1 above.  But
> it's an easy case: suppose that tempb is made up of 2 scalars with reverse
> storage order, then it's more complicated.  So memcpy where one of the types
> is aggregate with reverse storage order needs to be considered a storage order
> barrier, exactly like VIEW_CONVERT_EXPR:
>
>         case VIEW_CONVERT_EXPR:
>           temp.off = 0;
>           temp.reverse = storage_order_barrier_p (ref);
>           break;
>
> /* Return true if T is a storage order barrier, i.e. a VIEW_CONVERT_EXPR
>    that can modify the storage order of objects.  Note that, even if the
>    TYPE_REVERSE_STORAGE_ORDER flag is set on both the inner type and the
>    outer type, a VIEW_CONVERT_EXPR can modify the storage order because
>    it can change the partition of the aggregate object into scalars.  */
>
> static inline bool
> storage_order_barrier_p (const_tree t)
> {
>   if (TREE_CODE (t) != VIEW_CONVERT_EXPR)
>     return false;
>
>   if (AGGREGATE_TYPE_P (TREE_TYPE (t))
>       && TYPE_REVERSE_STORAGE_ORDER (TREE_TYPE (t)))
>     return true;
>
>   tree op = TREE_OPERAND (t, 0);
>
>   if (AGGREGATE_TYPE_P (TREE_TYPE (op))
>       && TYPE_REVERSE_STORAGE_ORDER (TREE_TYPE (op)))
>     return true;
>
>   return false;
> }
>
> hence the similar conditions, but I can add some more commentary.

OK, but all I was saying is that looking at the pointed-to type of
the address argument to the memcpy looks fragile to me.  For the
case cited above it would be better to look at the actual
reference we are looking up and not allowing memcpy handling
when that reference contains a storage order barrier?  Like
a && !contains_storage_order_barrier_p (vr->operands) on the whole block
like we do for the assign from constant/SSA value case?

> > Hum.  How does the pointer type matter at all for memcpy()?
> > Isn't the
> > issue just that we may not use the TYPE_REVERSE_STORAGE_ORDER
> > qualified type in the following case:
> >
> >       gimple *new_stmt;
> >       if (is_gimple_reg_type (TREE_TYPE (srcvar)))
> >         {
> >           tree tem = fold_const_aggregate_ref (srcvar);
> >           if (tem)
> >             srcvar = tem;
> >           if (! is_gimple_min_invariant (srcvar))
> >             {
> >               new_stmt = gimple_build_assign (NULL_TREE, srcvar);
> >               srcvar = create_tmp_reg_or_ssa_name (TREE_TYPE (srcvar),
> >                                                    new_stmt);
> >
> > ?
>
> Same rationale as above: if you want to rewrite the copy in the presence of an
> aggregate type with reverse storage order, you may need to split the operation
> into the appropriate pieces; one large access may be incorrect.

But the above cited case is the _only_ case we're possibly building an
assignment of a variable - so it's enough to assert that in the above case
destvar is not TYPE_REVERSE_STORAGE_ORDER?

The other cases all build an aggregate copy assignment with
a non-reverse-storage-order char[size] type which should be OK, no?
Note even for the above we probably would be fine if we'd made sure
to use a !TYPE_REVERSE_STORAGE_ORDER "qualified" type
for the access.  So the memcpy happens via a native order
load plus a native order store, exactly what memcpy does (in fact
the actual order does not matter unless the source and destination
order "agree")?  Thus maybe amend

      /* Make sure we are not copying using a floating-point mode or
         a type whose size possibly does not match its precision.  */
      if (FLOAT_MODE_P (TYPE_MODE (desttype))
          || TREE_CODE (desttype) == BOOLEAN_TYPE
          || TREE_CODE (desttype) == ENUMERAL_TYPE)
        desttype = bitwise_type_for_mode (TYPE_MODE (desttype));
      if (FLOAT_MODE_P (TYPE_MODE (srctype))
          || TREE_CODE (srctype) == BOOLEAN_TYPE
          || TREE_CODE (srctype) == ENUMERAL_TYPE)
        srctype = bitwise_type_for_mode (TYPE_MODE (srctype));

with || TYPE_REVERSE_STORAGE_ORDER (srctype) (same for desttype)?

Thanks,
Richard.

>
> --
> Eric Botcazou