Re: [PATCH/RFC] Start to use VIEW_CONVERT_EXPR more and start to fix PR 26069

[Diego Novillo <dnovillo at redhat dot com>]

Andrew Pinski wrote on 09/04/06 20:17:
> The problem with PR 26069 is that we removed address_of rtl which
> removed a small set of optimizations which were done.
> One example is:
> int f(short a)
> {o
>   unsigned short b = *(unsigned short*)&a;
>   return b;
> }
> We don't really need to store a in memory to get the value from a.
> 
> What this patch is three things:
> 1) Add to fold_indirect_ref_1, folding of *(unsigned short*)&a into
>    VIEW_CONVERT<unsigned short>a when the type sizes are the same and
>    the aliasing sets are the same.
> 2) Have the gimplifier swap which side of a MODIFY_EXPR, the VIEW_CONVERT_EXPR
>    is on.  This helps because otherwise a in "VIEW_CONVERT_EXPR<int> a = b;" needs
>    to be addressable.
> 3) Fix a bug in tree-ssa-operands which does not mark a in "VIEW_CONVERT <int> a = b;"
>    as addressable.
> 

> 2 is important for the following testcase:
> int f(short a)
> {
>   unsigned short b;
>   *(short*)&b = a;
>   return b;
> }
> 
> If we don't have 2 here, then we lose and still make b addressable
> and store it to the stack.
> 
> The one question I have about 1 is should we ignore the aliasing set and always
> do this transformation even though we know we turn undefined code into defined code?
> Right now I check the aliasing sets but I don't see there is a good reason to.

> This is only a small step in getting PR 26069 fixed because that bug has different
> sizes when dealing with the types.
> 
> OK for the mainline (yes the testcases above are a regression from 3.4.0)?
> Bootstrapped and tested on i686-linux-gnu.
> 
> Thanks,
> Andrew Pinski
> 

> +  if (!TYPE_VOLATILE (outertype)
> +      && simple_cst_equal (sizeouter, sizeinner)
> +      && (!get_alias_set (outertype)
> +          || get_alias_set (outertype) == get_alias_set (innertype)))
>
If we've warned then I have no problems in letting the folder ignore the
alias sets.  Undefined code is fair game.  Wait for stage 1 for this
one, though.


> +  /* Do the folding of the indirect reference so we can try to optimize
> +     VCEs.  */
> +  if (TREE_CODE (*to_p) == INDIRECT_REF)
> +      *to_p = fold_indirect_ref (*to_p);
> +
> +  /*  For VCE<type> a = b, it is better that we have a = VCE<othertype> b.
> +      Do this for types which don't have placeholder expressions in them.  */
> +  if (TREE_CODE (*to_p) == VIEW_CONVERT_EXPR 
> +      && !type_contains_placeholder_p (TREE_TYPE (TREE_OPERAND (*to_p, 0)))
> +      && simple_cst_equal (TYPE_SIZE (TREE_TYPE (*to_p)),
> +			   TYPE_SIZE (TREE_TYPE (TREE_OPERAND (*to_p, 0)))))
> +    {
> +      tree inner = TREE_OPERAND (*to_p, 0);
> +      tree otherside = *from_p;
> +      otherside = fold_build1 (VIEW_CONVERT_EXPR, TREE_TYPE (inner),
> +			       otherside);
> +      *to_p = inner;
> +      *from_p = otherside;
> +    }
> +
OK.

> Index: tree-ssa-operands.c
> ===================================================================
> --- tree-ssa-operands.c	(revision 116671)
> +++ tree-ssa-operands.c	(working copy)
> @@ -1843,6 +1843,11 @@ get_expr_operands (tree stmt, tree *expr
>  
>    switch (code)
>      {
> +    case VIEW_CONVERT_EXPR:
> +      if (flags & opf_is_def)
> +        add_to_addressable_set (TREE_OPERAND (expr, 0), &s_ann->addresses_taken);
> +      goto do_unary;
> +
>
Not OK.  This can't be right.  VCE does not take the address of its
operand.  Only ADDR_EXPR does.  I think you're papering over another
problem here.  The comment in tree.def about addressability applies to
VCE on the LHS and to the expression, not the operand.