[Bug tree-optimization/94216] New: [10 Regression] ICE in maybe_canonicalize_mem_ref_addr, at gimple-fold.c:4899 since r10-7237-g4e3d3e40726e1b68bf52fa205c68495124ea60b8

[Bug tree-optimization/94216] New: [10 Regression] ICE in maybe_canonicalize_mem_ref_addr, at gimple-fold.c:4899 since r10-7237-g4e3d3e40726e1b68bf52fa205c68495124ea60b8

[marxin at gcc dot gnu.org]

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

            Bug ID: 94216
           Summary: [10 Regression] ICE in
                    maybe_canonicalize_mem_ref_addr, at gimple-fold.c:4899
                    since
                    r10-7237-g4e3d3e40726e1b68bf52fa205c68495124ea60b8
           Product: gcc
           Version: 10.0
            Status: UNCONFIRMED
          Keywords: ice-on-valid-code
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: marxin at gcc dot gnu.org
                CC: rguenth at gcc dot gnu.org
  Target Milestone: ---

I see the following ICE isolated from ade package:

$ cat /tmp/comm.ii
template <int _Nm> struct A { typedef int _Type[_Nm]; };
template <int _Nm> struct B {
  typename A<_Nm>::_Type _M_elems;
  void operator[](int) { int a = *_M_elems; }
};
class C {
  struct D {
    using type = int *;
  };

public:
  using pointer = D::type;
};
class F {
public:
  using pointer = C::pointer;
  F(pointer);
};
struct G {
  int data;
};
template <int MaxDimensions> struct H {
  using dimensions_t = B<MaxDimensions>;
  dimensions_t dimensions;
  G mem;
};
template <int MaxDimensions, typename Allocator, typename DimT, typename
AlignT>
H<MaxDimensions> alloc_view(int, DimT, AlignT, Allocator) {
  H<MaxDimensions> b;
  b.dimensions[0];
  return b;
}
namespace memory {
template <typename> using DynMdView = H<6>;
}
class I {
  I();
  memory::DynMdView<void> m_view;
  F m_memory;
};
int c, d, e;
I::I() : m_view(alloc_view<6>(c, d, e, [] {})), m_memory(&m_view.mem.data) {}

$ ./xgcc -B. /tmp/comm.ii -c -g -O 
during IPA pass: inline
/tmp/comm.ii: In constructor ‘I::I()’:
/tmp/comm.ii:42:1: internal compiler error: in maybe_canonicalize_mem_ref_addr,
at gimple-fold.c:4899
   42 | I::I() : m_view(alloc_view<6>(c, d, e, [] {})),
m_memory(&m_view.mem.data) {}
      | ^
0xeae3c7 maybe_canonicalize_mem_ref_addr
        ../../gcc/gimple-fold.c:4899
0xeaedda fold_stmt_1
        ../../gcc/gimple-fold.c:5046
0xeafb25 fold_stmt(gimple_stmt_iterator*)
        ../../gcc/gimple-fold.c:5307
0x1219414 fold_marked_statements
        ../../gcc/tree-inline.c:5341
0x12278f3 optimize_inline_calls(tree_node*)
        ../../gcc/tree-inline.c:5447
0x1b56403 inline_transform(cgraph_node*)
        ../../gcc/ipa-inline-transform.c:722
0x10c09ba execute_one_ipa_transform_pass
        ../../gcc/passes.c:2233
0x10c09ba execute_all_ipa_transforms(bool)
        ../../gcc/passes.c:2272
0xd24fcb cgraph_node::expand()
        ../../gcc/cgraphunit.c:2276
0xd2609f expand_all_functions
        ../../gcc/cgraphunit.c:2454
0xd2609f symbol_table::compile()
        ../../gcc/cgraphunit.c:2804
0xd2872c symbol_table::compile()
        ../../gcc/cgraphunit.c:2717
0xd2872c symbol_table::finalize_compilation_unit()
        ../../gcc/cgraphunit.c:2984
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.

[Bug tree-optimization/94216] [10 Regression] ICE in maybe_canonicalize_mem_ref_addr, at gimple-fold.c:4899 since r10-7237-g4e3d3e40726e1b68bf52fa205c68495124ea60b8

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |10.0
   Last reconfirmed|                            |2020-03-18
      Known to work|                            |9.3.0
           Priority|P3                          |P1
             Status|UNCONFIRMED                 |NEW
      Known to fail|                            |10.0
     Ever confirmed|0                           |1

[Bug tree-optimization/94216] [10 Regression] ICE in maybe_canonicalize_mem_ref_addr, at gimple-fold.c:4899 since r10-7237-g4e3d3e40726e1b68bf52fa205c68495124ea60b8

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I wonder if we shouldn't do:
--- gcc/fold-const.c.jj 2020-03-18 12:47:36.000000000 +0100
+++ gcc/fold-const.c    2020-03-18 17:34:14.586455801 +0100
@@ -82,6 +82,7 @@ along with GCC; see the file COPYING3.
 #include "attribs.h"
 #include "tree-vector-builder.h"
 #include "vec-perm-indices.h"
+#include "tree-ssa.h"

 /* Nonzero if we are folding constants inside an initializer; zero
    otherwise.  */
@@ -10262,6 +10263,10 @@ fold_binary_loc (location_t loc, enum tr
   switch (code)
     {
     case MEM_REF:
+      STRIP_USELESS_TYPE_CONVERSION (arg0);
+      if (arg0 != op0)
+       return fold_build2 (MEM_REF, type, arg0, op1);
+
       /* MEM[&MEM[p, CST1], CST2] -> MEM[p, CST1 + CST2].  */
       if (TREE_CODE (arg0) == ADDR_EXPR
          && TREE_CODE (TREE_OPERAND (arg0, 0)) == MEM_REF)
to catch all similar issues.  Otherwise, we'd need to strip the useless type
conversion at least in the case which triggers this:
          return fold_build2 (MEM_REF, type,
                              build_fold_addr_expr (base),
                              int_const_binop (PLUS_EXPR, arg1,
                                               size_int (coffset)));
a few lines below this, where build_fold_addr_expr now returns a NOP_EXPR that
we really want to strip again, even when op0 wasn't a NOP_EXPR.

[Bug tree-optimization/94216] [10 Regression] ICE in maybe_canonicalize_mem_ref_addr, at gimple-fold.c:4899 since r10-7237-g4e3d3e40726e1b68bf52fa205c68495124ea60b8

[xerofoify at gmail dot com]

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

Nicholas Krause <xerofoify at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |xerofoify at gmail dot com

--- Comment #2 from Nicholas Krause <xerofoify at gmail dot com> ---
Jakub, 
I just tested your patch like this:
./gcc -B. comm.ii -c -g -O

as mentioned by Martin Liska's report. It does not crash now so this
should be fixed by it and the compiler was configured like:
Configured with: ../gcc/configure --enable-lanuages=c,c++ --enable-threads
-disable-multilibs --enable-checking=yes --prefix=/home/xerofoify/obdjir

[Bug tree-optimization/94216] [10 Regression] ICE in maybe_canonicalize_mem_ref_addr, at gimple-fold.c:4899 since r10-7237-g4e3d3e40726e1b68bf52fa205c68495124ea60b8

[jakub at gcc dot gnu.org]

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Nicholas Krause from comment #2)
> Jakub, 
> I just tested your patch like this:
> ./gcc -B. comm.ii -c -g -O
> 
> as mentioned by Martin Liska's report. It does not crash now so this
> should be fixed by it and the compiler was configured like:
> Configured with: ../gcc/configure --enable-lanuages=c,c++ --enable-threads
> -disable-multilibs --enable-checking=yes --prefix=/home/xerofoify/obdjir

What is this comment good for?  Of course I've tested my patch against that
testcase, what I haven't done (yet) is full bootstrap/regtest.

[Bug tree-optimization/94216] [10 Regression] ICE in maybe_canonicalize_mem_ref_addr, at gimple-fold.c:4899 since r10-7237-g4e3d3e40726e1b68bf52fa205c68495124ea60b8

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
Mine.

[Bug tree-optimization/94216] [10 Regression] ICE in maybe_canonicalize_mem_ref_addr, at gimple-fold.c:4899 since r10-7237-g4e3d3e40726e1b68bf52fa205c68495124ea60b8

[rguenth at gcc dot gnu.org]

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

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #1)
> I wonder if we shouldn't do:
> --- gcc/fold-const.c.jj	2020-03-18 12:47:36.000000000 +0100
> +++ gcc/fold-const.c	2020-03-18 17:34:14.586455801 +0100
> @@ -82,6 +82,7 @@ along with GCC; see the file COPYING3.
>  #include "attribs.h"
>  #include "tree-vector-builder.h"
>  #include "vec-perm-indices.h"
> +#include "tree-ssa.h"
>  
>  /* Nonzero if we are folding constants inside an initializer; zero
>     otherwise.  */
> @@ -10262,6 +10263,10 @@ fold_binary_loc (location_t loc, enum tr
>    switch (code)
>      {
>      case MEM_REF:
> +      STRIP_USELESS_TYPE_CONVERSION (arg0);

We already applied STRIP_NOPS to arg0

> +      if (arg0 != op0)
> +	return fold_build2 (MEM_REF, type, arg0, op1);
> +
>        /* MEM[&MEM[p, CST1], CST2] -> MEM[p, CST1 + CST2].  */
>        if (TREE_CODE (arg0) == ADDR_EXPR
>  	  && TREE_CODE (TREE_OPERAND (arg0, 0)) == MEM_REF)
> to catch all similar issues.  Otherwise, we'd need to strip the useless type
> conversion at least in the case which triggers this:
>           return fold_build2 (MEM_REF, type,
>                               build_fold_addr_expr (base),
>                               int_const_binop (PLUS_EXPR, arg1,
>                                                size_int (coffset)));
> a few lines below this, where build_fold_addr_expr now returns a NOP_EXPR
> that we really want to strip again, even when op0 wasn't a NOP_EXPR.

True.  But note there could be a not useless type conversion here, for
example for MEM<void (*)()> [&a] and void *a for example.  Here I think
the better fix is (again) to use build1 and then in case the base was
a MEM_REF recurse to the preceeding pattern.

I'm testing such a patch.

[Bug tree-optimization/94216] [10 Regression] ICE in maybe_canonicalize_mem_ref_addr, at gimple-fold.c:4899 since r10-7237-g4e3d3e40726e1b68bf52fa205c68495124ea60b8

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:73bc09fa8c6b973a928a599498caa66a25c8bc8d

commit r10-7272-g73bc09fa8c6b973a928a599498caa66a25c8bc8d
Author: Richard Biener <rguenther@suse.de>
Date:   Thu Mar 19 10:15:52 2020 +0100

    middle-end/94216 fix another build_fold_addr_expr use

    2020-03-19  Richard Biener  <rguenther@suse.de>

            PR middle-end/94216
            * fold-const.c (fold_binary_loc): Avoid using
            build_fold_addr_expr when we really want an ADDR_EXPR.

            * g++.dg/torture/pr94216.C: New testcase.

[Bug tree-optimization/94216] [10 Regression] ICE in maybe_canonicalize_mem_ref_addr, at gimple-fold.c:4899 since r10-7237-g4e3d3e40726e1b68bf52fa205c68495124ea60b8

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed.

[Bug tree-optimization/94216] [10 Regression] ICE in maybe_canonicalize_mem_ref_addr, at gimple-fold.c:4899 since r10-7237-g4e3d3e40726e1b68bf52fa205c68495124ea60b8

[jakub at gcc dot gnu.org]

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #5)
> (In reply to Jakub Jelinek from comment #1)
> > I wonder if we shouldn't do:
> > --- gcc/fold-const.c.jj	2020-03-18 12:47:36.000000000 +0100
> > +++ gcc/fold-const.c	2020-03-18 17:34:14.586455801 +0100
> > @@ -82,6 +82,7 @@ along with GCC; see the file COPYING3.
> >  #include "attribs.h"
> >  #include "tree-vector-builder.h"
> >  #include "vec-perm-indices.h"
> > +#include "tree-ssa.h"
> >  
> >  /* Nonzero if we are folding constants inside an initializer; zero
> >     otherwise.  */
> > @@ -10262,6 +10263,10 @@ fold_binary_loc (location_t loc, enum tr
> >    switch (code)
> >      {
> >      case MEM_REF:
> > +      STRIP_USELESS_TYPE_CONVERSION (arg0);
> 
> We already applied STRIP_NOPS to arg0

Though, if we don't want to strip non-useless conversions, that is wrong even
for the two special cases we have afterwards.
So, shouldn't case MEM_REF: start then with
      arg0 = op0;
      STRIP_USELESS_TYPE_CONVERSION (arg0);
      arg1 = op1;
?
Or fold_convert to the type of op0 if the type conversion isn't useless?
Also, isn't the arg1 handling incorrect or at least dangerous?
I mean, if it does int_const_binop (PLUS_EXPR, arg1, ...) in both cases
then it will have the type of arg1 which is op1 after STRIP_NOPS, so could have
completely different type.  One needs to hope that the last argument to
fold_binary_loc of MEM_REF will always be an INTEGER_CST from which nothing can
be stripped...

[Bug tree-optimization/94216] [10 Regression] ICE in maybe_canonicalize_mem_ref_addr, at gimple-fold.c:4899 since r10-7237-g4e3d3e40726e1b68bf52fa205c68495124ea60b8

[rguenther at suse dot de]

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

--- Comment #9 from rguenther at suse dot de <rguenther at suse dot de> ---
On Thu, 19 Mar 2020, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94216
> 
> --- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> (In reply to Richard Biener from comment #5)
> > (In reply to Jakub Jelinek from comment #1)
> > > I wonder if we shouldn't do:
> > > --- gcc/fold-const.c.jj	2020-03-18 12:47:36.000000000 +0100
> > > +++ gcc/fold-const.c	2020-03-18 17:34:14.586455801 +0100
> > > @@ -82,6 +82,7 @@ along with GCC; see the file COPYING3.
> > >  #include "attribs.h"
> > >  #include "tree-vector-builder.h"
> > >  #include "vec-perm-indices.h"
> > > +#include "tree-ssa.h"
> > >  
> > >  /* Nonzero if we are folding constants inside an initializer; zero
> > >     otherwise.  */
> > > @@ -10262,6 +10263,10 @@ fold_binary_loc (location_t loc, enum tr
> > >    switch (code)
> > >      {
> > >      case MEM_REF:
> > > +      STRIP_USELESS_TYPE_CONVERSION (arg0);
> > 
> > We already applied STRIP_NOPS to arg0
> 
> Though, if we don't want to strip non-useless conversions, that is wrong even
> for the two special cases we have afterwards.
> So, shouldn't case MEM_REF: start then with
>       arg0 = op0;
>       STRIP_USELESS_TYPE_CONVERSION (arg0);
>       arg1 = op1;
> ?

While we "abuse" fold_binary (MEM_REF,...) to make MEM_REFs valid
we still expect some basic hygiene there..

> Or fold_convert to the type of op0 if the type conversion isn't useless?
> Also, isn't the arg1 handling incorrect or at least dangerous?
> I mean, if it does int_const_binop (PLUS_EXPR, arg1, ...) in both cases
> then it will have the type of arg1 which is op1 after STRIP_NOPS, so could have
> completely different type.  One needs to hope that the last argument to
> fold_binary_loc of MEM_REF will always be an INTEGER_CST from which nothing can
> be stripped...

..for example INTEGER_CST 2nd argument (implicit in the use of
int_const_binop).  For the 2nd arg we could be more explicit and
instead of arg1 use op1.  Likewise we should probably use

      if (TREE_CODE (op0) == ADDR_EXPR
          && TREE_CODE (TREE_OPERAND (op0, 0)) == MEM_REF)

that is, we don't even expect to need to strip nops here.  I'll try
to bootstrap/test such changes to see where other possible issues
in fold_build_addr_expr callers lie...

[Bug tree-optimization/94216] [10 Regression] ICE in maybe_canonicalize_mem_ref_addr, at gimple-fold.c:4899 since r10-7237-g4e3d3e40726e1b68bf52fa205c68495124ea60b8

[rguenther at suse dot de]

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

--- Comment #10 from rguenther at suse dot de <rguenther at suse dot de> ---
On Thu, 19 Mar 2020, Richard Biener wrote:

> On Thu, 19 Mar 2020, jakub at gcc dot gnu.org wrote:
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94216
> > 
> > --- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> > (In reply to Richard Biener from comment #5)
> > > (In reply to Jakub Jelinek from comment #1)
> > > > I wonder if we shouldn't do:
> > > > --- gcc/fold-const.c.jj	2020-03-18 12:47:36.000000000 +0100
> > > > +++ gcc/fold-const.c	2020-03-18 17:34:14.586455801 +0100
> > > > @@ -82,6 +82,7 @@ along with GCC; see the file COPYING3.
> > > >  #include "attribs.h"
> > > >  #include "tree-vector-builder.h"
> > > >  #include "vec-perm-indices.h"
> > > > +#include "tree-ssa.h"
> > > >  
> > > >  /* Nonzero if we are folding constants inside an initializer; zero
> > > >     otherwise.  */
> > > > @@ -10262,6 +10263,10 @@ fold_binary_loc (location_t loc, enum tr
> > > >    switch (code)
> > > >      {
> > > >      case MEM_REF:
> > > > +      STRIP_USELESS_TYPE_CONVERSION (arg0);
> > > 
> > > We already applied STRIP_NOPS to arg0
> > 
> > Though, if we don't want to strip non-useless conversions, that is wrong even
> > for the two special cases we have afterwards.
> > So, shouldn't case MEM_REF: start then with
> >       arg0 = op0;
> >       STRIP_USELESS_TYPE_CONVERSION (arg0);
> >       arg1 = op1;
> > ?
> 
> While we "abuse" fold_binary (MEM_REF,...) to make MEM_REFs valid
> we still expect some basic hygiene there..
> 
> > Or fold_convert to the type of op0 if the type conversion isn't useless?
> > Also, isn't the arg1 handling incorrect or at least dangerous?
> > I mean, if it does int_const_binop (PLUS_EXPR, arg1, ...) in both cases
> > then it will have the type of arg1 which is op1 after STRIP_NOPS, so could have
> > completely different type.  One needs to hope that the last argument to
> > fold_binary_loc of MEM_REF will always be an INTEGER_CST from which nothing can
> > be stripped...
> 
> ..for example INTEGER_CST 2nd argument (implicit in the use of
> int_const_binop).  For the 2nd arg we could be more explicit and
> instead of arg1 use op1.  Likewise we should probably use
> 
>       if (TREE_CODE (op0) == ADDR_EXPR
>           && TREE_CODE (TREE_OPERAND (op0, 0)) == MEM_REF)
> 
> that is, we don't even expect to need to strip nops here.  I'll try
> to bootstrap/test such changes to see where other possible issues
> in fold_build_addr_expr callers lie...

That worked well without any further visible fallout.