[GOOGLE] Fix LIPO resolved node reference fixup

[GOOGLE] Fix LIPO resolved node reference fixup

[Teresa Johnson]

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

Re: [GOOGLE] Fix LIPO resolved node reference fixup

[Xinliang David Li]

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

Re: [GOOGLE] Fix LIPO resolved node reference fixup

[Teresa Johnson]

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

Re: [GOOGLE] Fix LIPO resolved node reference fixup

[Teresa Johnson]

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

Re: [GOOGLE] Fix LIPO resolved node reference fixup

[Xinliang David Li]

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