[Bug tree-optimization/69352] [6 Regression] profiledbootstrap failure with --with-build-config=bootstrap-lto

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69352

--- Comment #12 from Eric Botcazou  ---
[Sorry for the delay, email hiccup over the last couple of days]

> 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).

The design is that memory references are always accessed in the same storage
order, i.e. there will be no references to the same memory location differing
only by the storage order in the program; in other words, operand_equal_p need
not bother about REF_REVERSE_STORAGE_ORDER if everything works as designed.

Likewise for useless_type_conversion_p: TYPE_REVERSE_STORAGE_ORDER is a
property of canonical types and cannot differ between types with the same
canonical type.

[Bug tree-optimization/69352] [6 Regression] profiledbootstrap failure with --with-build-config=bootstrap-lto

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69352

--- Comment #13 from Jakub Jelinek  ---
(In reply to Eric Botcazou from comment #12)
> [Sorry for the delay, email hiccup over the last couple of days]
> 
> > 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).
> 
> The design is that memory references are always accessed in the same storage
> order, i.e. there will be no references to the same memory location
> differing only by the storage order in the program; in other words,
> operand_equal_p need not bother about REF_REVERSE_STORAGE_ORDER if
> everything works as designed.

How do you ensure that?  You could (perhaps through aliasing violation or union
or what) access the same memory slot through two different MEMs, and if one of
them has reverse and the other does not...

> Likewise for useless_type_conversion_p: TYPE_REVERSE_STORAGE_ORDER is a
> property of canonical types and cannot differ between types with the same
> canonical type.

Is TYPE_REVERSE_STORAGE_ORDER only used for aggregates, or even scalar types?
Because for the latter, useless_type_conversion_p usually only cares about
precision and signedness and nothing else, doesn't care about TYPE_CANONICAL.

[Bug tree-optimization/69352] [6 Regression] profiledbootstrap failure with --with-build-config=bootstrap-lto

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69352

--- Comment #14 from Eric Botcazou  ---
> How do you ensure that?  You could (perhaps through aliasing violation or
> union or what) access the same memory slot through two different MEMs, and
> if one of them has reverse and the other does not...

Indeed, you cannot use type punning to toggle the storage order, or else Bad
Things will happen...

> Is TYPE_REVERSE_STORAGE_ORDER only used for aggregates, or even scalar types?

Only for aggregate types.

[Bug tree-optimization/69352] [6 Regression] profiledbootstrap failure with --with-build-config=bootstrap-lto

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69352

Richard Biener  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED
   Assignee|unassigned at gcc dot gnu.org  |rguenth at gcc dot 
gnu.org

--- Comment #6 from Richard Biener  ---
Mine.

[Bug tree-optimization/69352] [6 Regression] profiledbootstrap failure with --with-build-config=bootstrap-lto

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69352

--- Comment #8 from Richard Biener  ---
(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.

[Bug tree-optimization/69352] [6 Regression] profiledbootstrap failure with --with-build-config=bootstrap-lto

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69352

Andrew Pinski  changed:

   What|Removed |Added

   Keywords||build, ice-on-valid-code
  Component|bootstrap   |tree-optimization
   Target Milestone|--- |6.0

[Bug tree-optimization/69352] [6 Regression] profiledbootstrap failure with --with-build-config=bootstrap-lto

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69352

--- Comment #7 from Jakub Jelinek  ---
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.

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).

[Bug tree-optimization/69352] [6 Regression] profiledbootstrap failure with --with-build-config=bootstrap-lto

[alalaw01 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69352

--- Comment #9 from alalaw01 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,

Agreed, yes. That would fix the bogus asserts, right - we would then only use
equal_mem_array_ref_p if size==max_size.

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

I think it should be OK to ignore differences in alias type for DOM
optimizations etc., indeed, which is where this was intended.

[Bug tree-optimization/69352] [6 Regression] profiledbootstrap failure with --with-build-config=bootstrap-lto

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69352

Richard Biener  changed:

   What|Removed |Added

 Status|ASSIGNED|RESOLVED
 Resolution|--- |FIXED

--- Comment #11 from Richard Biener  ---
Fixed.

[Bug tree-optimization/69352] [6 Regression] profiledbootstrap failure with --with-build-config=bootstrap-lto

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69352

--- Comment #10 from Richard Biener  ---
Author: rguenth
Date: Tue Jan 19 13:19:01 2016
New Revision: 232557

URL: https://gcc.gnu.org/viewcvs?rev=232557=gcc=rev
Log:
2016-01-19  Richard Biener  

PR tree-optimization/69352
* tree-ssa-scopedtables.c (avail_expr_hash): Check for size == -1.
(equal_mem_array_ref_p): Constrain size and max size properly.
Compare the reverse flag.

* gcc.dg/torture/pr69352.c: New testcase.

Added:
trunk/gcc/testsuite/gcc.dg/torture/pr69352.c
Modified:
trunk/gcc/ChangeLog
trunk/gcc/testsuite/ChangeLog
trunk/gcc/tree-ssa-scopedtables.c