Message ID | 20190701093216.spartrylpbbyun5v@kam.mff.cuni.cz |
---|---|
State | New |
Headers | show |
Series | Fix verifier ICE on CLOBBER of COMPONENT_REF | expand |
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
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)) {} +