Re: [PR 47382] We cannot simply fold OBJ_TYPE_REF at all in 4.6

[Richard Guenther <rguenther at suse dot de>]

On Tue, 25 Jan 2011, Martin Jambor wrote:

> Hi,
> 
> PR 47382 is another devirtualization issue but at least this time I
> think I did not cause it.  The problem is that even when I reverted
> gimple-folding of OBJ_TYPE_REF to what is being done in 4.5, the IL
> now looks different and thus we still miscompile stuff.
> 
> For the PR testcase, 4.5 early inliner produces:
> 
> int main() ()
> {
>   struct A * this.1;
>   int (*__vtbl_ptr_type) (void) * D.1820;
>   int (*__vtbl_ptr_type) (void) D.1819;
>   struct B b;
>   int D.1793;
> 
> <bb 2>:
>   b.D.1713._vptr.A = &_ZTV1A[2];
>   b.D.1713._vptr.A = &_ZTV1B[2];
>   D.1793_1 = 0;
>   b.D.1713._vptr.A = &_ZTV1B[2];
>   this.1_5 = &b.D.1713;
>   this.1_5->_vptr.A = &_ZTV1A[2];
>   D.1820_6 = this.1_5->_vptr.A;
>   D.1819_7 = *D.1820_6;
>   OBJ_TYPE_REF(D.1819_7;this.1_5->0) (this.1_5);
> 
> <L2>:
> <L3>:
>   return D.1793_1;
> 
> }
> 
> Whereas in 4.6 the IL (after disabling O_T_R folding) looks like:
> 
> int main() ()
> {
>   int (*__vtbl_ptr_type) (void) * D.1819;
>   int (*__vtbl_ptr_type) (void) D.1818;
>   struct B b;
>   int D.1794;
> 
> <bb 2>:
>   MEM[(struct A *)&b]._vptr.A = &_ZTV1A[2];
>   b.D.1714._vptr.A = &_ZTV1B[2];
>   D.1794_1 = 0;
>   b.D.1714._vptr.A = &_ZTV1B[2];
>   MEM[(struct A *)&b]._vptr.A = &_ZTV1A[2];
>   D.1819_5 = MEM[(struct A *)&b]._vptr.A;
>   D.1818_6 = *D.1819_5;
>   OBJ_TYPE_REF(D.1818_6;&b->0) (&b);
> 
> <L2>:
> <L3>:
>   return D.1794_1;
> 
> }
> 
> Notice the subtle but important difference in the 1st (counted from
> zero) argument of the OBJ_TYPE_REFs.
> 
> O_T_R folding as we've done it so far (since I first looked at the
> source) relied on the fact that &DECL means we do not call a virtual
> method of an ancestor but of the whole (outer-most) object, which
> avoids many problems, including those with changing dynamic type.
> However this apparently does not hold true for the current trunk.

As I mentioned repeatedly pointer types have no semantic meaning ;)

> Since the optimizations that lead to this seem all valid and useful in
> other circumstances, I guess that we simply cannot fold OBJ_TYPE_REFs
> without scanning for dynamic type changes at all and so the patch
> below disables it.
> 
> Nevertheless, we do have a testcase in our testsuite
> (g++.dg/opt/devirt1.C), that actually checks that this folding is
> being done and so I have xfailed it.  Without LTO, our only chance of
> optimizing code like this (with constructor in a different compilation
> unit) is to use types and so I have had a look at what my patches I
> have not committed do with this source.  Unfortunately, they bail out
> when they try to make sure that the destination of the call is not an
> complex type of thunk (only this-adjusting thunks are not too complex
> to be represented in the call graph now).  This is done using call
> graph nodes and since the call target does not have one, I give up.
> 
> Is there any way of determining if the function declaration extracted
> from BINFO_VIRTUALS is a thunk?  I use TREE_PURPOSE to get the delta
> by which to adjust this pointer already, do all thunks have a non-zero
> delta in TREE_PURPOSE in BINFO_VIRTUALS?  It would be better to fail
> to devirtualize only for thunks (even simple ones) than for all
> methods...
> 
> Anyway, the patch below successfully bootstrapped and tested on
> x86_64-linux.  OK for trunk?

Ok.

Thanks,
Richard.

> Thanks,
> 
> Martin
> 
> 
> 2011-01-24  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR tree-optimization/47382
> 	* gimple-fold.c (gimple_fold_obj_type_ref_call): Removed.
> 	(gimple_fold_call): Do not call gimple_fold_obj_type_ref_call.
> 
> 	* testsuite/g++.dg/torture/pr47382.C: New test.
> 	* testsuite/g++.dg/opt/devirt1.C: Xfail.
> 
> Index: icln/gcc/gimple-fold.c
> ===================================================================
> --- icln.orig/gcc/gimple-fold.c
> +++ icln/gcc/gimple-fold.c
> @@ -1443,38 +1443,6 @@ gimple_adjust_this_by_delta (gimple_stmt
>    gimple_call_set_arg (call_stmt, 0, tmp);
>  }
>  
> -/* Fold a call statement to OBJ_TYPE_REF to a direct call, if possible.  GSI
> -   determines the statement, generating new statements is allowed only if
> -   INPLACE is false.  Return true iff the statement was changed.  */
> -
> -static bool
> -gimple_fold_obj_type_ref_call (gimple_stmt_iterator *gsi)
> -{
> -  gimple stmt = gsi_stmt (*gsi);
> -  tree ref = gimple_call_fn (stmt);
> -  tree obj = OBJ_TYPE_REF_OBJECT (ref);
> -  tree binfo, fndecl, delta;
> -  HOST_WIDE_INT token;
> -
> -  if (TREE_CODE (obj) != ADDR_EXPR)
> -    return false;
> -  obj = TREE_OPERAND (obj, 0);
> -  if (!DECL_P (obj)
> -      || TREE_CODE (TREE_TYPE (obj)) != RECORD_TYPE)
> -    return false;
> -  binfo = TYPE_BINFO (TREE_TYPE (obj));
> -  if (!binfo)
> -    return false;
> -
> -  token = tree_low_cst (OBJ_TYPE_REF_TOKEN (ref), 1);
> -  fndecl = gimple_get_virt_mehtod_for_binfo (token, binfo, &delta, false);
> -  if (!fndecl)
> -    return false;
> -  gcc_assert (integer_zerop (delta));
> -  gimple_call_set_fndecl (stmt, fndecl);
> -  return true;
> -}
> -
>  /* Attempt to fold a call statement referenced by the statement iterator GSI.
>     The statement may be replaced by another statement, e.g., if the call
>     simplifies to a constant value. Return true if any changes were made.
> @@ -1500,17 +1468,6 @@ gimple_fold_call (gimple_stmt_iterator *
>  	  return true;
>  	}
>      }
> -  else
> -    {
> -      /* ??? Should perhaps do this in fold proper.  However, doing it
> -         there requires that we create a new CALL_EXPR, and that requires
> -         copying EH region info to the new node.  Easier to just do it
> -         here where we can just smash the call operand.  */
> -      callee = gimple_call_fn (stmt);
> -      if (TREE_CODE (callee) == OBJ_TYPE_REF)
> -	return gimple_fold_obj_type_ref_call (gsi);
> -    }
> -
>    return false;
>  }
>  
> Index: icln/gcc/testsuite/g++.dg/torture/pr47382.C
> ===================================================================
> --- /dev/null
> +++ icln/gcc/testsuite/g++.dg/torture/pr47382.C
> @@ -0,0 +1,30 @@
> +// { dg-do run }
> +
> +extern "C" void abort ();
> +
> +struct A
> +{
> +  inline ~A ();
> +  virtual void foo () {}
> +};
> +
> +struct B : A
> +{
> +  virtual void foo () { abort(); }
> +};
> +
> +static inline void middleman (A *a)
> +{
> +  a->foo ();
> +}
> +
> +inline A::~A ()
> +{
> +  middleman (this);
> +}
> +
> +int main ()
> +{
> +   B b;
> +   return 0;
> +}
> Index: icln/gcc/testsuite/g++.dg/opt/devirt1.C
> ===================================================================
> --- icln.orig/gcc/testsuite/g++.dg/opt/devirt1.C
> +++ icln/gcc/testsuite/g++.dg/opt/devirt1.C
> @@ -1,6 +1,6 @@
>  // { dg-do compile }
>  // { dg-options "-O" }
> -// { dg-final { scan-assembler "xyzzy" } }
> +// { dg-final { scan-assembler "xyzzy" { xfail *-*-* } } }
>  
>  struct S { S(); virtual void xyzzy(); };
>  inline void foo(S *s) { s->xyzzy(); }
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex