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 <mli...@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 <mli...@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
