On Thu, Sep 24, 2015 at 11:07 AM, Simon Dardis <simon.dar...@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.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)))));
>> }
