* [GOOGLE] Fix LIPO resolved node reference fixup
@ 2014-10-24 17:48 Teresa Johnson
2014-10-24 18:28 ` Xinliang David Li
0 siblings, 1 reply; 5+ messages in thread
From: Teresa Johnson @ 2014-10-24 17:48 UTC (permalink / raw)
To: David Li; +Cc: gcc-patches
This patch makes a fix to the reference fixups performed after LIPO
node resolution, to better handle the case where we are updating the
base address of a reference.
Fixes google benchmark and passes regression tests. Ok for google/4_9?
Thanks,
Teresa
2014-10-24 Teresa Johnson <tejohnson@google.com>
Google ref b/18110567.
* cgraphbuild.c (get_base_address_expr): New function.
(fixup_ref): Update the op expression for new base address.
Index: cgraphbuild.c
===================================================================
--- cgraphbuild.c (revision 216667)
+++ cgraphbuild.c (working copy)
@@ -665,13 +665,35 @@ record_references_in_initializer (tree decl, bool
pointer_set_destroy (visited_nodes);
}
+/* Similar to get_base_address but returns the ADDR_EXPR pointing
+ to the base address corresponding to T. */
+
+static tree
+get_base_address_expr (tree t)
+{
+ while (handled_component_p (t))
+ t = TREE_OPERAND (t, 0);
+
+ if ((TREE_CODE (t) == MEM_REF
+ || TREE_CODE (t) == TARGET_MEM_REF)
+ && TREE_CODE (TREE_OPERAND (t, 0)) == ADDR_EXPR)
+ return TREE_OPERAND (t, 0);
+
+ return NULL_TREE;
+}
+
/* Update any function decl references in base ADDR of operand OP to refer to
the resolved node. */
static bool
fixup_ref (gimple, tree addr, tree op)
{
+ tree orig_addr = addr;
addr = get_base_address (addr);
+ /* If the address was changed, update the operand OP to be the
+ ADDR_EXPR pointing to the new base address. */
+ if (orig_addr != addr)
+ op = get_base_address_expr (orig_addr);
if (addr && TREE_CODE (addr) == FUNCTION_DECL)
{
gcc_assert (TREE_CODE (op) == ADDR_EXPR);
--
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [GOOGLE] Fix LIPO resolved node reference fixup
2014-10-24 17:48 [GOOGLE] Fix LIPO resolved node reference fixup Teresa Johnson
@ 2014-10-24 18:28 ` Xinliang David Li
2014-10-24 19:32 ` Teresa Johnson
0 siblings, 1 reply; 5+ messages in thread
From: Xinliang David Li @ 2014-10-24 18:28 UTC (permalink / raw)
To: Teresa Johnson; +Cc: gcc-patches
When orgin_addr == addr, is there a guarantee that this assert:
gcc_assert (TREE_OPERAND (op,0) == addr);
is always true?
David
On Fri, Oct 24, 2014 at 10:21 AM, Teresa Johnson <tejohnson@google.com> wrote:
> This patch makes a fix to the reference fixups performed after LIPO
> node resolution, to better handle the case where we are updating the
> base address of a reference.
>
> Fixes google benchmark and passes regression tests. Ok for google/4_9?
>
> Thanks,
> Teresa
>
> 2014-10-24 Teresa Johnson <tejohnson@google.com>
>
> Google ref b/18110567.
> * cgraphbuild.c (get_base_address_expr): New function.
> (fixup_ref): Update the op expression for new base address.
>
> Index: cgraphbuild.c
> ===================================================================
> --- cgraphbuild.c (revision 216667)
> +++ cgraphbuild.c (working copy)
> @@ -665,13 +665,35 @@ record_references_in_initializer (tree decl, bool
> pointer_set_destroy (visited_nodes);
> }
>
> +/* Similar to get_base_address but returns the ADDR_EXPR pointing
> + to the base address corresponding to T. */
> +
> +static tree
> +get_base_address_expr (tree t)
> +{
> + while (handled_component_p (t))
> + t = TREE_OPERAND (t, 0);
> +
> + if ((TREE_CODE (t) == MEM_REF
> + || TREE_CODE (t) == TARGET_MEM_REF)
> + && TREE_CODE (TREE_OPERAND (t, 0)) == ADDR_EXPR)
> + return TREE_OPERAND (t, 0);
> +
> + return NULL_TREE;
> +}
> +
> /* Update any function decl references in base ADDR of operand OP to refer to
> the resolved node. */
>
> static bool
> fixup_ref (gimple, tree addr, tree op)
> {
> + tree orig_addr = addr;
> addr = get_base_address (addr);
> + /* If the address was changed, update the operand OP to be the
> + ADDR_EXPR pointing to the new base address. */
> + if (orig_addr != addr)
> + op = get_base_address_expr (orig_addr);
> if (addr && TREE_CODE (addr) == FUNCTION_DECL)
> {
> gcc_assert (TREE_CODE (op) == ADDR_EXPR);
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [GOOGLE] Fix LIPO resolved node reference fixup
2014-10-24 18:28 ` Xinliang David Li
@ 2014-10-24 19:32 ` Teresa Johnson
2014-10-27 14:35 ` Teresa Johnson
0 siblings, 1 reply; 5+ messages in thread
From: Teresa Johnson @ 2014-10-24 19:32 UTC (permalink / raw)
To: Xinliang David Li; +Cc: gcc-patches
On Fri, Oct 24, 2014 at 10:55 AM, Xinliang David Li <davidxl@google.com> wrote:
> When orgin_addr == addr, is there a guarantee that this assert:
>
> gcc_assert (TREE_OPERAND (op,0) == addr);
>
> is always true?
It should be, that is the assumption of the code that we are trying to
enforce with the assert.
Teresa
>
> David
>
>
>
> On Fri, Oct 24, 2014 at 10:21 AM, Teresa Johnson <tejohnson@google.com> wrote:
>> This patch makes a fix to the reference fixups performed after LIPO
>> node resolution, to better handle the case where we are updating the
>> base address of a reference.
>>
>> Fixes google benchmark and passes regression tests. Ok for google/4_9?
>>
>> Thanks,
>> Teresa
>>
>> 2014-10-24 Teresa Johnson <tejohnson@google.com>
>>
>> Google ref b/18110567.
>> * cgraphbuild.c (get_base_address_expr): New function.
>> (fixup_ref): Update the op expression for new base address.
>>
>> Index: cgraphbuild.c
>> ===================================================================
>> --- cgraphbuild.c (revision 216667)
>> +++ cgraphbuild.c (working copy)
>> @@ -665,13 +665,35 @@ record_references_in_initializer (tree decl, bool
>> pointer_set_destroy (visited_nodes);
>> }
>>
>> +/* Similar to get_base_address but returns the ADDR_EXPR pointing
>> + to the base address corresponding to T. */
>> +
>> +static tree
>> +get_base_address_expr (tree t)
>> +{
>> + while (handled_component_p (t))
>> + t = TREE_OPERAND (t, 0);
>> +
>> + if ((TREE_CODE (t) == MEM_REF
>> + || TREE_CODE (t) == TARGET_MEM_REF)
>> + && TREE_CODE (TREE_OPERAND (t, 0)) == ADDR_EXPR)
>> + return TREE_OPERAND (t, 0);
>> +
>> + return NULL_TREE;
>> +}
>> +
>> /* Update any function decl references in base ADDR of operand OP to refer to
>> the resolved node. */
>>
>> static bool
>> fixup_ref (gimple, tree addr, tree op)
>> {
>> + tree orig_addr = addr;
>> addr = get_base_address (addr);
>> + /* If the address was changed, update the operand OP to be the
>> + ADDR_EXPR pointing to the new base address. */
>> + if (orig_addr != addr)
>> + op = get_base_address_expr (orig_addr);
>> if (addr && TREE_CODE (addr) == FUNCTION_DECL)
>> {
>> gcc_assert (TREE_CODE (op) == ADDR_EXPR);
>>
>>
>> --
>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
--
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [GOOGLE] Fix LIPO resolved node reference fixup
2014-10-24 19:32 ` Teresa Johnson
@ 2014-10-27 14:35 ` Teresa Johnson
2014-10-27 16:33 ` Xinliang David Li
0 siblings, 1 reply; 5+ messages in thread
From: Teresa Johnson @ 2014-10-27 14:35 UTC (permalink / raw)
To: Xinliang David Li; +Cc: gcc-patches
Here is the new patch that walks op looking for the reference to addr.
Passes internal benchmarks and regression tests. Ok for google/4_9?
Thanks,
Teresa
2014-10-27 Teresa Johnson <tejohnson@google.com>
Google ref b/18110567.
* cgraphbuild.c (fixup_all_refs_1): New function.
(fixup_all_refs): Ditto.
(fixup_ref): Walk tree to find and replace ref.
Index: cgraphbuild.c
===================================================================
--- cgraphbuild.c (revision 216735)
+++ cgraphbuild.c (working copy)
@@ -665,6 +665,45 @@ record_references_in_initializer (tree decl, bool
pointer_set_destroy (visited_nodes);
}
+typedef struct _fixup_decl_info {
+ tree orig_decl;
+ tree new_decl;
+} fixup_decl_info;
+
+/* Check the tree at TP to see if it contains the original decl stored in
+ DATA and if so replace it with the new decl. If original decl is
+ found set WALK_SUBTREES to 0 so the subtree under TP is not traversed.
+ Returns the updated parent tree T or NULL if no update performed. */
+
+static tree
+fixup_all_refs_1 (tree *tp, int *walk_subtrees, void *data)
+{
+ tree t = *tp;
+ fixup_decl_info *info = (fixup_decl_info *) data;
+
+ /* The original function decl is always the first tree operand. */
+ if (TREE_OPERAND (t,0) == info->orig_decl)
+ {
+ TREE_OPERAND (t,0) = info->new_decl;
+ *walk_subtrees = 0;
+ return t;
+ }
+ return NULL_TREE;
+}
+
+/* Walk the whole tree rooted at TP and invoke fixup_all_refs_1 to
+ replace any references to the original decl with the new decl
+ stored in INFO. */
+
+static inline void
+fixup_all_refs (tree *tp, fixup_decl_info *info)
+{
+ tree t = walk_tree (tp, fixup_all_refs_1, info, NULL);
+ /* This is invoked when we found the original decl, so we expect
+ to have replaced a reference. */
+ gcc_assert (t != NULL_TREE);
+}
+
/* Update any function decl references in base ADDR of operand OP to refer to
the resolved node. */
@@ -674,13 +713,16 @@ fixup_ref (gimple, tree addr, tree op)
addr = get_base_address (addr);
if (addr && TREE_CODE (addr) == FUNCTION_DECL)
{
- gcc_assert (TREE_CODE (op) == ADDR_EXPR);
- gcc_assert (TREE_OPERAND (op,0) == addr);
struct cgraph_node *real_callee;
real_callee = cgraph_lipo_get_resolved_node (addr);
if (addr == real_callee->decl)
return false;
- TREE_OPERAND (op,0) = real_callee->decl;
+ /* We need to locate and update the tree operand within OP
+ that contains ADDR and update it to the real callee's decl. */
+ fixup_decl_info info;
+ info.orig_decl = addr;
+ info.new_decl = real_callee->decl;
+ fixup_all_refs (&op, &info);
}
return false;
}
On Fri, Oct 24, 2014 at 11:46 AM, Teresa Johnson <tejohnson@google.com> wrote:
> On Fri, Oct 24, 2014 at 10:55 AM, Xinliang David Li <davidxl@google.com> wrote:
>> When orgin_addr == addr, is there a guarantee that this assert:
>>
>> gcc_assert (TREE_OPERAND (op,0) == addr);
>>
>> is always true?
>
> It should be, that is the assumption of the code that we are trying to
> enforce with the assert.
>
> Teresa
>
>>
>> David
>>
>>
>>
>> On Fri, Oct 24, 2014 at 10:21 AM, Teresa Johnson <tejohnson@google.com> wrote:
>>> This patch makes a fix to the reference fixups performed after LIPO
>>> node resolution, to better handle the case where we are updating the
>>> base address of a reference.
>>>
>>> Fixes google benchmark and passes regression tests. Ok for google/4_9?
>>>
>>> Thanks,
>>> Teresa
>>>
>>> 2014-10-24 Teresa Johnson <tejohnson@google.com>
>>>
>>> Google ref b/18110567.
>>> * cgraphbuild.c (get_base_address_expr): New function.
>>> (fixup_ref): Update the op expression for new base address.
>>>
>>> Index: cgraphbuild.c
>>> ===================================================================
>>> --- cgraphbuild.c (revision 216667)
>>> +++ cgraphbuild.c (working copy)
>>> @@ -665,13 +665,35 @@ record_references_in_initializer (tree decl, bool
>>> pointer_set_destroy (visited_nodes);
>>> }
>>>
>>> +/* Similar to get_base_address but returns the ADDR_EXPR pointing
>>> + to the base address corresponding to T. */
>>> +
>>> +static tree
>>> +get_base_address_expr (tree t)
>>> +{
>>> + while (handled_component_p (t))
>>> + t = TREE_OPERAND (t, 0);
>>> +
>>> + if ((TREE_CODE (t) == MEM_REF
>>> + || TREE_CODE (t) == TARGET_MEM_REF)
>>> + && TREE_CODE (TREE_OPERAND (t, 0)) == ADDR_EXPR)
>>> + return TREE_OPERAND (t, 0);
>>> +
>>> + return NULL_TREE;
>>> +}
>>> +
>>> /* Update any function decl references in base ADDR of operand OP to refer to
>>> the resolved node. */
>>>
>>> static bool
>>> fixup_ref (gimple, tree addr, tree op)
>>> {
>>> + tree orig_addr = addr;
>>> addr = get_base_address (addr);
>>> + /* If the address was changed, update the operand OP to be the
>>> + ADDR_EXPR pointing to the new base address. */
>>> + if (orig_addr != addr)
>>> + op = get_base_address_expr (orig_addr);
>>> if (addr && TREE_CODE (addr) == FUNCTION_DECL)
>>> {
>>> gcc_assert (TREE_CODE (op) == ADDR_EXPR);
>>>
>>>
>>> --
>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
--
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [GOOGLE] Fix LIPO resolved node reference fixup
2014-10-27 14:35 ` Teresa Johnson
@ 2014-10-27 16:33 ` Xinliang David Li
0 siblings, 0 replies; 5+ messages in thread
From: Xinliang David Li @ 2014-10-27 16:33 UTC (permalink / raw)
To: Teresa Johnson; +Cc: gcc-patches
ok.
thanks,
David
On Mon, Oct 27, 2014 at 7:33 AM, Teresa Johnson <tejohnson@google.com> wrote:
> Here is the new patch that walks op looking for the reference to addr.
>
> Passes internal benchmarks and regression tests. Ok for google/4_9?
>
> Thanks,
> Teresa
>
> 2014-10-27 Teresa Johnson <tejohnson@google.com>
>
> Google ref b/18110567.
> * cgraphbuild.c (fixup_all_refs_1): New function.
> (fixup_all_refs): Ditto.
> (fixup_ref): Walk tree to find and replace ref.
>
> Index: cgraphbuild.c
> ===================================================================
> --- cgraphbuild.c (revision 216735)
> +++ cgraphbuild.c (working copy)
> @@ -665,6 +665,45 @@ record_references_in_initializer (tree decl, bool
> pointer_set_destroy (visited_nodes);
> }
>
> +typedef struct _fixup_decl_info {
> + tree orig_decl;
> + tree new_decl;
> +} fixup_decl_info;
> +
> +/* Check the tree at TP to see if it contains the original decl stored in
> + DATA and if so replace it with the new decl. If original decl is
> + found set WALK_SUBTREES to 0 so the subtree under TP is not traversed.
> + Returns the updated parent tree T or NULL if no update performed. */
> +
> +static tree
> +fixup_all_refs_1 (tree *tp, int *walk_subtrees, void *data)
> +{
> + tree t = *tp;
> + fixup_decl_info *info = (fixup_decl_info *) data;
> +
> + /* The original function decl is always the first tree operand. */
> + if (TREE_OPERAND (t,0) == info->orig_decl)
> + {
> + TREE_OPERAND (t,0) = info->new_decl;
> + *walk_subtrees = 0;
> + return t;
> + }
> + return NULL_TREE;
> +}
> +
> +/* Walk the whole tree rooted at TP and invoke fixup_all_refs_1 to
> + replace any references to the original decl with the new decl
> + stored in INFO. */
> +
> +static inline void
> +fixup_all_refs (tree *tp, fixup_decl_info *info)
> +{
> + tree t = walk_tree (tp, fixup_all_refs_1, info, NULL);
> + /* This is invoked when we found the original decl, so we expect
> + to have replaced a reference. */
> + gcc_assert (t != NULL_TREE);
> +}
> +
> /* Update any function decl references in base ADDR of operand OP to refer to
> the resolved node. */
>
> @@ -674,13 +713,16 @@ fixup_ref (gimple, tree addr, tree op)
> addr = get_base_address (addr);
> if (addr && TREE_CODE (addr) == FUNCTION_DECL)
> {
> - gcc_assert (TREE_CODE (op) == ADDR_EXPR);
> - gcc_assert (TREE_OPERAND (op,0) == addr);
> struct cgraph_node *real_callee;
> real_callee = cgraph_lipo_get_resolved_node (addr);
> if (addr == real_callee->decl)
> return false;
> - TREE_OPERAND (op,0) = real_callee->decl;
> + /* We need to locate and update the tree operand within OP
> + that contains ADDR and update it to the real callee's decl. */
> + fixup_decl_info info;
> + info.orig_decl = addr;
> + info.new_decl = real_callee->decl;
> + fixup_all_refs (&op, &info);
> }
> return false;
> }
>
>
> On Fri, Oct 24, 2014 at 11:46 AM, Teresa Johnson <tejohnson@google.com> wrote:
>> On Fri, Oct 24, 2014 at 10:55 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> When orgin_addr == addr, is there a guarantee that this assert:
>>>
>>> gcc_assert (TREE_OPERAND (op,0) == addr);
>>>
>>> is always true?
>>
>> It should be, that is the assumption of the code that we are trying to
>> enforce with the assert.
>>
>> Teresa
>>
>>>
>>> David
>>>
>>>
>>>
>>> On Fri, Oct 24, 2014 at 10:21 AM, Teresa Johnson <tejohnson@google.com> wrote:
>>>> This patch makes a fix to the reference fixups performed after LIPO
>>>> node resolution, to better handle the case where we are updating the
>>>> base address of a reference.
>>>>
>>>> Fixes google benchmark and passes regression tests. Ok for google/4_9?
>>>>
>>>> Thanks,
>>>> Teresa
>>>>
>>>> 2014-10-24 Teresa Johnson <tejohnson@google.com>
>>>>
>>>> Google ref b/18110567.
>>>> * cgraphbuild.c (get_base_address_expr): New function.
>>>> (fixup_ref): Update the op expression for new base address.
>>>>
>>>> Index: cgraphbuild.c
>>>> ===================================================================
>>>> --- cgraphbuild.c (revision 216667)
>>>> +++ cgraphbuild.c (working copy)
>>>> @@ -665,13 +665,35 @@ record_references_in_initializer (tree decl, bool
>>>> pointer_set_destroy (visited_nodes);
>>>> }
>>>>
>>>> +/* Similar to get_base_address but returns the ADDR_EXPR pointing
>>>> + to the base address corresponding to T. */
>>>> +
>>>> +static tree
>>>> +get_base_address_expr (tree t)
>>>> +{
>>>> + while (handled_component_p (t))
>>>> + t = TREE_OPERAND (t, 0);
>>>> +
>>>> + if ((TREE_CODE (t) == MEM_REF
>>>> + || TREE_CODE (t) == TARGET_MEM_REF)
>>>> + && TREE_CODE (TREE_OPERAND (t, 0)) == ADDR_EXPR)
>>>> + return TREE_OPERAND (t, 0);
>>>> +
>>>> + return NULL_TREE;
>>>> +}
>>>> +
>>>> /* Update any function decl references in base ADDR of operand OP to refer to
>>>> the resolved node. */
>>>>
>>>> static bool
>>>> fixup_ref (gimple, tree addr, tree op)
>>>> {
>>>> + tree orig_addr = addr;
>>>> addr = get_base_address (addr);
>>>> + /* If the address was changed, update the operand OP to be the
>>>> + ADDR_EXPR pointing to the new base address. */
>>>> + if (orig_addr != addr)
>>>> + op = get_base_address_expr (orig_addr);
>>>> if (addr && TREE_CODE (addr) == FUNCTION_DECL)
>>>> {
>>>> gcc_assert (TREE_CODE (op) == ADDR_EXPR);
>>>>
>>>>
>>>> --
>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
>>
>>
>>
>> --
>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-10-27 16:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-24 17:48 [GOOGLE] Fix LIPO resolved node reference fixup Teresa Johnson
2014-10-24 18:28 ` Xinliang David Li
2014-10-24 19:32 ` Teresa Johnson
2014-10-27 14:35 ` Teresa Johnson
2014-10-27 16:33 ` Xinliang David Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).