Re: Fix devirtualiation in expanded thunks

[Sudakshina Das <Sudi dot Das at arm dot com>]

Hi Jan

On 21/12/18 7:20 PM, Jan Hubicka wrote:
> Hi,
> this patch fixes polymorphic call analysis in thunks.  Unlike normal
> methods, thunks take THIS pointer offsetted by a known constant. This
> needs t be compensated for when calculating address of outer type.
>
> Bootstrapped/regtested x86_64-linux, also tested with Firefox where this
> bug trigger misoptimization in spellchecker.  I plan to backport it to
> release branches soon.
>
> Honza
>
> 	PR ipa/88561
> 	* ipa-polymorphic-call.c
> 	(ipa_polymorphic_call_context::ipa_polymorphic_call_context): Handle
> 	arguments of thunks correctly.
> 	(ipa_polymorphic_call_context::get_dynamic_context): Be ready for
> 	NULL instance pinter.
> 	* lto-cgraph.c (lto_output_node): Always stream thunk info.
> 	* g++.dg/tree-prof/devirt.C: New testcase.
> Index: ipa-polymorphic-call.c
> ===================================================================
> --- ipa-polymorphic-call.c	(revision 267325)
> +++ ipa-polymorphic-call.c	(working copy)
> @@ -995,9 +995,22 @@ ipa_polymorphic_call_context::ipa_polymo
>   	{
>   	  outer_type
>   	     = TYPE_MAIN_VARIANT (TREE_TYPE (TREE_TYPE (base_pointer)));
> +	  cgraph_node *node = cgraph_node::get (current_function_decl);
>   	  gcc_assert (TREE_CODE (outer_type) == RECORD_TYPE
>   		      || TREE_CODE (outer_type) == UNION_TYPE);
>   
> +	  /* Handle the case we inlined into a thunk.  In this case
> +	     thunk has THIS pointer of type bar, but it really receives
> +	     address to its base type foo which sits in bar at
> +	     0-thunk.fixed_offset.  It starts with code that adds
> +	     think.fixed_offset to the pointer to compensate for this.
> +
> +	     Because we walked all the way to the begining of thunk, we now
> +	     see pointer &bar-thunk.fixed_offset and need to compensate
> +	     for it.  */
> +	  if (node->thunk.fixed_offset)
> +	    offset -= node->thunk.fixed_offset * BITS_PER_UNIT;
> +
>   	  /* Dynamic casting has possibly upcasted the type
>   	     in the hiearchy.  In this case outer type is less
>   	     informative than inner type and we should forget
> @@ -1005,7 +1018,11 @@ ipa_polymorphic_call_context::ipa_polymo
>   	  if ((otr_type
>   	       && !contains_type_p (outer_type, offset,
>   				    otr_type))
> -	      || !contains_polymorphic_type_p (outer_type))
> +	      || !contains_polymorphic_type_p (outer_type)
> +	      /* If we compile thunk with virtual offset, the THIS pointer
> +		 is adjusted by unknown value.  We can't thus use outer info
> +		 at all.  */
> +	      || node->thunk.virtual_offset_p)
>   	    {
>   	      outer_type = NULL;
>   	      if (instance)
> @@ -1030,7 +1047,15 @@ ipa_polymorphic_call_context::ipa_polymo
>   	      maybe_in_construction = false;
>   	    }
>   	  if (instance)
> -	    *instance = base_pointer;
> +	    {
> +	      /* If method is expanded thunk, we need to apply thunk offset
> +		 to instance pointer.  */
> +	      if (node->thunk.virtual_offset_p
> +		  || node->thunk.fixed_offset)
> +		*instance = NULL;
> +	      else
> +	        *instance = base_pointer;
> +	    }
>   	  return;
>   	}
>         /* Non-PODs passed by value are really passed by invisible
> @@ -1547,6 +1572,9 @@ ipa_polymorphic_call_context::get_dynami
>     HOST_WIDE_INT instance_offset = offset;
>     tree instance_outer_type = outer_type;
>   
> +  if (!instance)
> +    return false;
> +
>     if (otr_type)
>       otr_type = TYPE_MAIN_VARIANT (otr_type);
>   
> Index: lto-cgraph.c
> ===================================================================
> --- lto-cgraph.c	(revision 267325)
> +++ lto-cgraph.c	(working copy)
> @@ -547,7 +547,11 @@ lto_output_node (struct lto_simple_outpu
>     streamer_write_bitpack (&bp);
>     streamer_write_data_stream (ob->main_stream, section, strlen (section) + 1);
>   
> -  if (node->thunk.thunk_p)
> +  /* Stream thunk info always because we use it in
> +     ipa_polymorphic_call_context::ipa_polymorphic_call_context
> +     to properly interpret THIS pointers for thunks that has been converted
> +     to Gimple.  */
> +  if (node->definition)
>       {
>         streamer_write_uhwi_stream
>   	 (ob->main_stream,
> @@ -1295,7 +1299,7 @@ input_node (struct lto_file_decl_data *f
>     if (section)
>       node->set_section_for_node (section);
>   
> -  if (node->thunk.thunk_p)
> +  if (node->definition)
>       {
>         int type = streamer_read_uhwi (ib);
>         HOST_WIDE_INT fixed_offset = streamer_read_uhwi (ib);
> Index: testsuite/g++.dg/tree-prof/devirt.C
> ===================================================================
> --- testsuite/g++.dg/tree-prof/devirt.C	(nonexistent)
> +++ testsuite/g++.dg/tree-prof/devirt.C	(working copy)
> @@ -0,0 +1,123 @@
> +/* { dg-options "-O3 -fdump-tree-dom3" } */
> +struct nsISupports
> +{
> +  virtual int QueryInterface (const int &aIID, void **aInstancePtr) = 0;
> +  virtual __attribute__((noinline, noclone)) unsigned AddRef (void) = 0;
> +  virtual unsigned Release (void) = 0;
> +};
> +
> +struct nsIObserver : public nsISupports
> +{
> +  virtual int Observe (nsISupports * aSubject, const char *aTopic, const unsigned short *aData) = 0;
> +};
> +
> +struct nsISupportsWeakReference : public nsISupports
> +{
> +  virtual int GetWeakReference (void **_retval) = 0;
> +};
> +
> +struct nsSupportsWeakReference : public nsISupportsWeakReference
> +{
> +  nsSupportsWeakReference () : mProxy (0) {}
> +  virtual int GetWeakReference (void **_retval) override { return 0; }
> +  ~nsSupportsWeakReference () {}
> +  void NoticeProxyDestruction () { mProxy = nullptr; }
> +  void *mProxy;
> +  void ClearWeakReferences ();
> +  bool HasWeakReferences () const { return !!mProxy; }
> +};
> +
> +struct mozIPersonalDictionary : public nsISupports
> +{
> +  virtual int Load (void) = 0;
> +  virtual int Save (void) = 0;
> +  virtual int GetWordList (void **aWordList) = 0;
> +  virtual int Check (const int &word, bool * _retval) = 0;
> +  virtual int AddWord (const int &word) = 0;
> +  virtual int RemoveWord (const int &word) = 0;
> +  virtual int IgnoreWord (const int &word) = 0;
> +  virtual int EndSession (void) = 0;
> +};
> +
> +struct mozPersonalDictionary final
> +  : public mozIPersonalDictionary, public nsIObserver, public nsSupportsWeakReference
> +{
> +  virtual int QueryInterface (const int &aIID, void **aInstancePtr) override;
> +  virtual __attribute__((noinline, noclone)) unsigned AddRef (void) override;
> +  virtual unsigned Release (void) override;
> +  unsigned long mRefCnt;
> +  virtual int Load (void) override { return 0; }
> +  virtual int Save (void) override { return 0; }
> +  virtual int GetWordList (void **aWordList) override { return 0; }
> +  virtual int Check (const int &word, bool * _retval) override { return 0; }
> +  virtual int AddWord (const int &word) override { return 0; }
> +  virtual int RemoveWord (const int &word) override { return 0; }
> +  virtual int IgnoreWord (const int &word) override { return 0; }
> +  virtual int EndSession (void) override { return 0; }
> +  virtual int Observe (nsISupports * aSubject, const char *aTopic, const unsigned short *aData) override { return 0; }
> +  mozPersonalDictionary () : mRefCnt(0) {}
> +  int Init () { return 0; }
> +  virtual ~mozPersonalDictionary () {}
> +  bool mIsLoaded;
> +  bool mSavePending;
> +  void *mFile;
> +  char mMonitor[96];
> +  char mMonitorSave[96];
> +  char mDictionaryTable[32];
> +  char mIgnoreTable[32];
> +};
> +
> +unsigned
> +mozPersonalDictionary::AddRef (void)
> +{
> +  unsigned count = ++mRefCnt;
> +  return count;
> +}
> +
> +unsigned
> +mozPersonalDictionary::Release (void)
> +{
> +  unsigned count = --mRefCnt;
> +  if (count == 0)
> +    {
> +      mRefCnt = 1;
> +      delete (this);
> +      return 0;
> +    }
> +  return count;
> +}
> +
> +int
> +mozPersonalDictionary::QueryInterface (const int &aIID, void **aInstancePtr)
> +{
> +  nsISupports *foundInterface;
> +  if (aIID == 122)
> +    foundInterface = static_cast <mozIPersonalDictionary *>(this);
> +  else
> +    foundInterface = static_cast <nsISupportsWeakReference *>(this);
> +  int status;
> +  foundInterface->AddRef ();
> +  *aInstancePtr = foundInterface;
> +  return status;
> +}
> +
> +__attribute__((noipa)) int
> +foo (nsISupports *p, const int &i)
> +{
> +  void *q;
> +  return p->QueryInterface (i, &q);
> +}
> +
> +int
> +main ()
> +{
> +  mozPersonalDictionary m;
> +  int j = 123;
> +  for (int i = 0; i < 100000; i++)
> +    foo (static_cast <nsISupportsWeakReference *>(&m), j);
> +  if (m.mRefCnt != 100000)
> +    __builtin_abort ();
> +}
> +
This patch causes failures on aarch64-none-linux-gnu and 
arm-none-linux-gnueabihf

UNRESOLVED: g++.dg/tree-prof/devirt.C scan-ipa-dump-times dom3 "3" 
folding virtual function call to virtual unsigned int 
mozPersonalDictionary::AddRef
UNRESOLVED: g++.dg/tree-prof/devirt.C scan-ipa-dump-times dom3 "3" 
folding virtual function call to virtual unsigned int 
mozPersonalDictionary::_ZThn16

With

g++.dg/tree-prof/devirt.C: dump file does not exist

I had missed this one earlier thinking its the same with the LTO test 
failures and only realized this after the backport commit yesterday by 
Martin.

Thanks

Sudi

> +/* { dg-final-use-not-autofdo { scan-ipa-dump-times 3 "folding virtual function call to virtual unsigned int mozPersonalDictionary::_ZThn16" "dom3" } } */
> +/* { dg-final-use-not-autofdo { scan-ipa-dump-times 3 "folding virtual function call to virtual unsigned int mozPersonalDictionary::AddRef" "dom3" } } */