RE: Predictive commoning leads to register to register moves through memory.

[Simon Dardis]

It seems to me that extending operand_equal_p to deal with the
MEM_REF/ARRAY_REF case could help.

Your recent change of:

          if (TREE_CODE (arg0) == MEM_REF
              && DECL_P (arg1)
              && TREE_CODE (TREE_OPERAND (arg0, 0)) == ADDR_EXPR
              && TREE_OPERAND (TREE_OPERAND (arg0, 0), 0) == arg1
              && integer_zerop (TREE_OPERAND (arg0, 1)))
            return 1;
          else if (TREE_CODE (arg1) == MEM_REF
                   && DECL_P (arg0)
                   && TREE_CODE (TREE_OPERAND (arg1, 0)) == ADDR_EXPR
                   && TREE_OPERAND (TREE_OPERAND (arg1, 0), 0) == arg0
                   && integer_zerop (TREE_OPERAND (arg1, 1)))
            return 1;
          return 0;

only seems to be handing the case of 'X' and MEM_REF[&X + 0]. Do you think
changing this to be 'return addr_eq_p (arg0, arg1);' where addr_eq_p handles 
cases of ARRAY_(RANGED_)REF, COMPONENT_REF by checking offset as well
would be useful? 

I've put together an initial version which splits out change to operand_equal_p
into its own predicate for just IDing cases of base objects and the above
mentioned addr_eq_p. Also in that patch is the change for mem_attrs_eq_p as
in that case the offset is not part of the TREE expression, so it has to be 
handled
differently.

Thoughts?

Thanks,
Simon

-----Original Message-----
From: Richard Biener [mailto:richard.guent...@gmail.com] 
Sent: 22 September 2015 12:12
To: Simon Dardis
Cc: Jeff Law; gcc@gcc.gnu.org
Subject: Re: Predictive commoning leads to register to register moves through 
memory.

On Tue, Sep 22, 2015 at 12:45 PM, Simon Dardis <simon.dar...@imgtec.com> wrote:
> I took an attempt at addressing this through the RTL GCSE pass. This 
> attempt tweaks mem_attrs_eq_p to return true if its comparing something like 
> poly+8 and MEM [&poly + 8].
>
> Is this a more suitable approach?

I actually recently modified stmt_kills_ref_p for a similar reason...
not that I liked that vey much.
I think the more suitable approach would be to not have both forms if they are 
actually equal.
Of course that's way more work.

So splitting out a function that handles sematic equality compare of MEM_EXPR 
sounds good to me.
No comments on your actual implementation yet, but I think we should do it in a 
way to be re-usable by stmt_kills_ref_p.

Richard.


> Thanks,
> Simon
>
> +/* Return true if p and q reference the same location by the same name but
> +   through VAR_DECL and MEM_REF.  */
> +
> +static bool
> +mem_locations_match_p (const struct mem_attrs *p, const struct 
> +mem_attrs *q) {
> +  HOST_WIDE_INT var_offset;
> +  tree var, memref;
> +
> +  if (p->expr == NULL_TREE || q->expr == NULL_TREE)
> +    return false;
> +
> +  if (TREE_CODE (p->expr) == MEM_REF && TREE_CODE (q->expr) == VAR_DECL)
> +    {
> +      var = q->expr;
> +      var_offset = q->offset;
> +      memref = p->expr;
> +    }
> +  else if (TREE_CODE (q->expr) == MEM_REF && TREE_CODE (p->expr) == VAR_DECL)
> +    {
> +      var = p->expr;
> +      var_offset = p->offset;
> +      memref = q->expr;
> +    }
> +  else
> +    return false;
> +
> +  if (TREE_OPERAND (TREE_OPERAND (memref, 0), 0) != var)
> +    return false;
> +
> +  if (TREE_TYPE (TREE_TYPE (var)) != TREE_TYPE (memref))
> +    return false;
> +
> +  tree offset = TREE_OPERAND (memref, 1);  if ((TREE_CODE (offset) == 
> + INTEGER_CST && tree_fits_shwi_p (offset)
> +      && tree_to_shwi (offset) == var_offset)
> +      || offset == NULL_TREE && var_offset == 0)
> +    return true;
> +
> +  return false;
> +
> +}
> +
> /* Return true if the given memory attributes are equal.  */
>
>  bool
> @@ -254,16 +298,16 @@ mem_attrs_eq_p (const struct mem_attrs *p, const struct 
> mem_attrs *q)
>      return false;
>    return (p->alias == q->alias
>           && p->offset_known_p == q->offset_known_p
> -         && (!p->offset_known_p || p->offset == q->offset)
>           && p->size_known_p == q->size_known_p
>           && (!p->size_known_p || p->size == q->size)
>           && p->align == q->align
>           && p->addrspace == q->addrspace
> -         && (p->expr == q->expr
> -             || (p->expr != NULL_TREE && q->expr != NULL_TREE
> -                 && operand_equal_p (p->expr, q->expr, 0))));
> +         && (mem_locations_match_p (p, q)
> +            || (!p->offset_known_p || p->offset == q->offset)
> +                && (p->expr == q->expr
> +                    || (p->expr != NULL_TREE && q->expr != NULL_TREE
> +                        && operand_equal_p (p->expr, q->expr, 0)))));
>  }

gcse.patch
Description: gcse.patch