Re: Fix devirtualiation in expanded thunks

[Martin Liška <mliska at suse dot cz>]

On 12/28/18 12:19 PM, Sudakshina Das wrote:
> 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

Thanks for heads up. I'm going to install following obvious fix.

Martin

> 
> 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" } } */

>From 25ad262ed646778a3ea39b56a58a9bcf6d9f9c52 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Mon, 31 Dec 2018 15:06:40 +0100
Subject: [PATCH] Fix scan pattern of a test-case.

gcc/testsuite/ChangeLog:

2018-12-31  Martin Liska  <mliska@suse.cz>

	* g++.dg/tree-prof/devirt.C: Fix scan pattern and test options.
---
 gcc/testsuite/g++.dg/tree-prof/devirt.C | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gcc/testsuite/g++.dg/tree-prof/devirt.C b/gcc/testsuite/g++.dg/tree-prof/devirt.C
index 05c9a26e7a4..86cba41452e 100644
--- a/gcc/testsuite/g++.dg/tree-prof/devirt.C
+++ b/gcc/testsuite/g++.dg/tree-prof/devirt.C
@@ -1,4 +1,4 @@
-/* { dg-options "-O3 -fdump-tree-dom3" } */
+/* { dg-options "-O3 -fdump-tree-dom3-details" } */
 struct nsISupports
 {
   virtual int QueryInterface (const int &aIID, void **aInstancePtr) = 0;
@@ -119,5 +119,5 @@ main ()
     __builtin_abort ();
 }
 
-/* { 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" } } */
+/* { dg-final-use-not-autofdo { scan-tree-dump-times "folding virtual function call to virtual unsigned int mozPersonalDictionary::_ZThn16" 3 "dom3" } } */
+/* { dg-final-use-not-autofdo { scan-tree-dump-times "folding virtual function call to virtual unsigned int mozPersonalDictionary::AddRef" 3 "dom3" } } */
-- 
2.20.1