https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69352
--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #7)
> There are various bugs in the r232508 change.
> The
> gcc_assert (sz0 == sz1);
> gcc_assert (max0 == max1);
> gcc_assert (rev0 == rev1);
> asserts are clearly bogus, while for compatible type I bet size will be
> always the same, maximum size can be arbitrary (it will be either equal to
> size, then it is a fixed access, or it will be larger, then it is a variable
> access), and the reverse stuff looks weird (e.g. I think the lack of
> REF_REVERSE_STORAGE_ORDER testing in operand_equal_p is a bug). For a
> variable access, even if you remove the above max{0,1} assert, I think you
> would happily equate say a[i] and a[j] ARRAY_REFs, because they have the
> same off (likely 0) and max (likely size of array in bits). Another problem
> I see in the
> return equal_mem_array_ref_p (expr0->ops.single.rhs,
> expr1->ops.single.rhs)
> || operand_equal_p (expr0->ops.single.rhs,
> expr1->ops.single.rhs, 0);
> case; under some conditions you decide to hash the MEM_REF/ARRAY_REFs as
> MEM_REF , hash of base, offset and size, so you should use the same
> conditions to decide if you use equal_mem_array_ref_p or operand_equal_p,
> using the other one if you hashed it differently will just lead to hard to
> maintain randomness - the expressions are hashed using one property, and so
> the other equality function depends only on whether they happen to appear in
> the same hash bucket anyway or not, usually not. Fixing this should fix
> also the variable accesses - you'd then try to use the
> get_ref_base_and_extent stuff only for the fixed accesses and use
> operand_equal_p otherwise. Plus, I'm not sure in what places this hashing
> is used, I'm worried you might hash MEM_REFs with different alias types for
> the accesses as equal, which for some uses might be fine, if you are not
> trying to replace one with another etc., but for other cases it might lead
> to wrong-code.
See the patch I posted which fixes the variable index issue. operand_equal_p
should always work but yes, for compile-time things may be improved (I
originally suggested to use the SCCVN memory ref storing/hashing and I guess
we should try to commonize this for GCC 7).
As DOM is never materializing memory refs but just replace it with an
earlier result the alias thing is not an issue - it might just end up
hiding undefined behavior.
> CCing Eric on whether REF_REVERSE_STORAGE_ORDER should be checked in
> operand_equal_p and whether TYPE_REVERSE_STORAGE_ORDER doesn't have to be
> tested in useless_type_conversion_p (perhaps the latter is ok, if it is not
> possible for two aggregates with the same TYPE_CANONICAL to have different
> TYPE_REVERSE_STORAGE_ORDER).
On the REF_REVERSE_STORAGE_ORDER issue I have no idea. I suppose we should
compare it for equality rather than asserting its equality.
