Fix verifier ICE on CLOBBER of COMPONENT_REF

Hi,
the testcase makes inliner to substitute RESULT_DECL by COMPONENT_REF
inside CLOBBER statement. This is not allowed and leads to ICE in
verifier (while I think we should fix code to support this).
This patch simply makes inliner to watch for this case and do not remap
the statement at all.

The testcase works for 32bit only which is bit unfortunate. For 64bit
build NRV does not happen, i did not look into why.

OK?
Honza

	* tree-inline.c (remap_gimple_stmt): Do not subtitute handled components
	to clobber of return value.
	* g++.dg/lto/pr90990_0.C: New testcase.

On Mon, 1 Jul 2019, Jan Hubicka wrote:

> Hi,
> the testcase makes inliner to substitute RESULT_DECL by COMPONENT_REF
> inside CLOBBER statement. This is not allowed and leads to ICE in
> verifier (while I think we should fix code to support this).
> This patch simply makes inliner to watch for this case and do not remap
> the statement at all.
> 
> The testcase works for 32bit only which is bit unfortunate. For 64bit
> build NRV does not happen, i did not look into why.
> 
> OK?

Hmm, a bit ugly and seemingly in a "wrong place" but I cannot think
of a better solution.

Thus OK.

We indeed might reconsider the CLOBBER restriction - it's from
the time they were used for stack slot sharing but now they are
re-purposed...

Thanks,
Richard.

> Honza
> 
> 	* tree-inline.c (remap_gimple_stmt): Do not subtitute handled components
> 	to clobber of return value.
> 	* g++.dg/lto/pr90990_0.C: New testcase.
> Index: tree-inline.c
> ===================================================================
> --- tree-inline.c	(revision 272846)
> +++ tree-inline.c	(working copy)
> @@ -1757,6 +1757,18 @@ remap_gimple_stmt (gimple *stmt, copy_bo
>  		return NULL;
>  	    }
>  	}
> +     
> +      /* We do not allow CLOBBERs of handled components.  In case
> +	 returned value is stored via such handled component, remove
> +	 the clobber so stmt verifier is happy.  */
> +      if (gimple_clobber_p (stmt)
> +	  && TREE_CODE (gimple_assign_lhs (stmt)) == RESULT_DECL)
> +	{
> +	  tree remapped = remap_decl (gimple_assign_lhs (stmt), id);
> +	  if (!DECL_P (remapped)
> +	      && TREE_CODE (remapped) != MEM_REF)
> +	    return NULL;
> +	}
>  
>        if (gimple_debug_bind_p (stmt))
>  	{
> Index: testsuite/g++.dg/lto/pr90990_0.C
> ===================================================================
> --- testsuite/g++.dg/lto/pr90990_0.C	(nonexistent)
> +++ testsuite/g++.dg/lto/pr90990_0.C	(working copy)
> @@ -0,0 +1,31 @@
> +// { dg-lto-do link }
> +/* { dg-extra-ld-options {  -r -nostdlib } } */
> +class A {
> +public:
> +  float m_floats;
> +  A() {}
> +};
> +class B {
> +public:
> +  A operator[](int);
> +};
> +class C {
> +  B m_basis;
> +
> +public:
> +  A operator()(A) {
> +    m_basis[1] = m_basis[2];
> +    A a;
> +    return a;
> +  }
> +};
> +class D {
> +public:
> +  C m_fn1();
> +};
> +class F {
> +  A m_pivotInB;
> +  F(D &, const A &);
> +};
> +F::F(D &p1, const A &p2) : m_pivotInB(p1.m_fn1()(p2)) {}
> +
>

> On Mon, 1 Jul 2019, Jan Hubicka wrote:
> 
> > Hi,
> > the testcase makes inliner to substitute RESULT_DECL by COMPONENT_REF
> > inside CLOBBER statement. This is not allowed and leads to ICE in
> > verifier (while I think we should fix code to support this).
> > This patch simply makes inliner to watch for this case and do not remap
> > the statement at all.
> > 
> > The testcase works for 32bit only which is bit unfortunate. For 64bit
> > build NRV does not happen, i did not look into why.
> > 
> > OK?
> 
> Hmm, a bit ugly and seemingly in a "wrong place" but I cannot think
> of a better solution.
> 
> Thus OK.
> 
> We indeed might reconsider the CLOBBER restriction - it's from
> the time they were used for stack slot sharing but now they are
> re-purposed...

Agreed, I think we want to relax the restriction - it is useful to kill
part of structure.  But I will go with the fix first :)

Honza