This is the mail archive of the gcc@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: Predictive commoning leads to register to register moves through memory.


On Thu, Sep 24, 2015 at 11:07 AM, Simon Dardis <Simon.Dardis@imgtec.com> wrote:
> 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?

Yes, I think so.  Note that a more general addr_eq_p implementation would
simply use get_inner_reference and compare the results (its implementation
has the drawback of building GENERIC expression trees for non-constant
offset parts, there is also get_addr_base_and_unit_offset which avoids this
but cannot handle non-constant offsets).

Note the caller I did the change above for would also be happy knowing
about offsetted (with known constant offset) bases.

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

   return (p->alias == q->alias
-         && p->offset_known_p == q->offset_known_p
-         && (!p->offset_known_p || p->offset == q->offset)
+         && offsets_eq_p (p, q)

previous code treated both !offset_known_p as 'true', you're now treating it as
'false'.  I think the function really tests for equality of mem_attrs and thus
all callers need to be audited on whether they really need equal mem_attrs
or just partly equal bits.  Thus I think changing mem_attrs_eq is dangerous.

@@ -334,7 +366,8 @@
          && p->addrspace == q->addrspace
          && (p->expr == q->expr
              || (p->expr != NULL_TREE && q->expr != NULL_TREE
-                 && operand_equal_p (p->expr, q->expr, 0))));
+                 && (operand_equal_p (p->expr, q->expr, 0)
+                     || base_object_eq_p (p->expr, q->expr)))));

you can just pass OEP_ADDRESS_OF to operand_equal_p then, no?
So no need to export base_object_eq_p.

Why did you need offsets_eq_p at all?

+  if (p->expr != NULL_TREE && q->expr != NULL_TREE)
+    {
+      if (TREE_CODE (p->expr) == MEM_REF)
+       {
+         if (TREE_CODE (TREE_OPERAND (p->expr, 1)) == INTEGER_CST)
+           return !compare_tree_int (TREE_OPERAND (p->expr, 1), q->offset);

MEM_REF operand 1 is always an INTEGER_CST btw.  Also p can still
have a non-zero p->offset so you can't just compare the MEM_REF offset
with q->offset and be done.  In fact the MEM_REF offset has no relation
to the MEM_ATTR offset so I'm not sure how to make sense of that function.

Richard.

> Thanks,
> Simon
>
> -----Original Message-----
> From: Richard Biener [mailto:richard.guenther@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.Dardis@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)))));
>>  }


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