[Bug tree-optimization/107828] New: tree-inlining would generate SSA with incorrect def stmt

[Bug tree-optimization/107828] New: tree-inlining would generate SSA with incorrect def stmt

[fxue at os dot amperecomputing.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107828

            Bug ID: 107828
           Summary: tree-inlining would generate SSA with incorrect def
                    stmt
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: fxue at os dot amperecomputing.com
  Target Milestone: ---

In the function "remap_gimple_op_r" of tree-inlining.cc:

  ...

  if (TREE_CODE (*tp) == SSA_NAME)
    {
      *tp = remap_ssa_name (*tp, id);
      *walk_subtrees = 0;
      if (is_lhs)
         SSA_NAME_DEF_STMT (*tp) = wi_p->stmt;
      return NULL;
    }

The definition statement of original "*tp" may be same as wi_p->stmt. So, we
will end up with a statement that it is pointed by both old and new SSA name,
while the old one should have been reclaimed.

And this happens when some parameter needs to be adjusted during inline
versioning as:

remap_gimple_stmt()

{
  ...
  if (id->param_body_adjs)
    {
      ...
      if (!gimple_seq_empty_p (extra_stmts))
        {
          memset (&wi, 0, sizeof (wi));
          wi.info = id;
          for (gimple_stmt_iterator egsi = gsi_start (extra_stmts);
               !gsi_end_p (egsi);
               gsi_next (&egsi))
            walk_gimple_op (gsi_stmt (egsi), remap_gimple_op_r, &wi);
                                             ^^^^^^^^^^^^^^^^^

            ...
        }
    }
   ...
}

[Bug tree-optimization/107828] tree-inlining would generate SSA with incorrect def stmt

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107828

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-11-23
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |marxin at gcc dot gnu.org
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Martin Liška <marxin at gcc dot gnu.org> ---
@Honza?

[Bug tree-optimization/107828] tree-inlining would generate SSA with incorrect def stmt

[jamborm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107828

--- Comment #2 from Martin Jambor <jamborm at gcc dot gnu.org> ---
I don't see any correctness issue here.  It is true that when IPA-SRA
needs to create a temporary SSA_NAME for an assignment with
VIEW_CONVERT_EXPR (that is the only statement that extra_stmts can
contain at the moment), the SSA_NAME then get's re-mapped to another
while woe could have just re-use the existing one, it is already
created within the new function.

We could use the knowledge that extra_stmts is really only this one
statement or nothing and avoid the remapping but that would be at the
expense of universality and extensibility of the interface between
tree-inline.cc and ipa-param-manipulations.cc.

Please let me know if I am missing something (and preferably with a
test-case that demonstrates the problem).

[Bug tree-optimization/107828] tree-inlining would generate SSA with incorrect def stmt

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107828

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|NEW                         |RESOLVED

--- Comment #3 from Martin Liška <marxin at gcc dot gnu.org> ---
I tend to close this as Martin provided an answer.

[Bug tree-optimization/107828] tree-inlining would generate SSA with incorrect def stmt

[fxue at os dot amperecomputing.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107828

Feng Xue <fxue at os dot amperecomputing.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|INVALID                     |---
             Status|RESOLVED                    |WAITING

--- Comment #4 from Feng Xue <fxue at os dot amperecomputing.com> ---
(In reply to Martin Jambor from comment #2)
> I don't see any correctness issue here.  It is true that when IPA-SRA
> needs to create a temporary SSA_NAME for an assignment with
> VIEW_CONVERT_EXPR (that is the only statement that extra_stmts can
> contain at the moment), the SSA_NAME then get's re-mapped to another
> while woe could have just re-use the existing one, it is already
> created within the new function.
> 
> We could use the knowledge that extra_stmts is really only this one
> statement or nothing and avoid the remapping but that would be at the
> expense of universality and extensibility of the interface between
> tree-inline.cc and ipa-param-manipulations.cc.
> 
> Please let me know if I am missing something (and preferably with a
> test-case that demonstrates the problem).

Sorry for late response.

It is hard to compose a simple test case, since this could only be exposed with
our own private code. To reproduce it is not that straightforward, but we could
forcefully do that with a minor change to
"ipa_param_body_adjustments::modify_assignment" in ipa-param-manipulation.cc.

diff --git a/gcc/ipa-param-manipulation.cc b/gcc/ipa-param-manipulation.cc
index cee0e23f946..202c0fda32b 100644
--- a/gcc/ipa-param-manipulation.cc
+++ b/gcc/ipa-param-manipulation.cc
@@ -1787,7 +1787,7 @@ ipa_param_body_adjustments::modify_assignment (gimple
*stmt,
   any = modify_expression (lhs_p, false);
   any |= modify_expression (rhs_p, false);
   if (any
-      && !useless_type_conversion_p (TREE_TYPE (*lhs_p), TREE_TYPE (*rhs_p)))
+      && true /* !useless_type_conversion_p (TREE_TYPE (*lhs_p), TREE_TYPE
(*rhs_p)) */)
     {
       if (TREE_CODE (*rhs_p) == CONSTRUCTOR)
        {
@@ -1805,7 +1805,14 @@ ipa_param_body_adjustments::modify_assignment (gimple
*stmt,
                                          *rhs_p);
          tree tmp = force_gimple_operand (new_rhs, extra_stmts, true,
                                           NULL_TREE);
-         gimple_assign_set_rhs1 (stmt, tmp);
+
+         tree tmp1 = make_ssa_name (tmp);
+         gimple *copy = gimple_build_assign (tmp1, tmp);
+
+         auto gsi = gsi_last (*extra_stmts);
+         gsi_insert_after_without_update (&gsi, copy, GSI_NEW_STMT);
+
+         gimple_assign_set_rhs1 (stmt, tmp1);
        }
       return true;
     }

The purpose of above code piece is to add trivial ssa copy statement to make
the below "extra_stmts" not empty:

remap_gimple_stmt()

{
  ...
  if (id->param_body_adjs)
    {
      ...
      if (!gimple_seq_empty_p (extra_stmts))
        {
          memset (&wi, 0, sizeof (wi));
          wi.info = id;
          for (gimple_stmt_iterator egsi = gsi_start (extra_stmts);
               !gsi_end_p (egsi);
               gsi_next (&egsi))
            walk_gimple_op (gsi_stmt (egsi), remap_gimple_op_r, &wi);
                                             ^^^^^^^^^^^^^^^^^

            ...
        }
    }
   ...
}

Now, we use the case gcc.dg/ipa/ipa-sra-1.c, and option is "-O3 -flto
-fipa-sra". Trace into "remap_gimple_op_r" as above, check remapping on copy:

<&0x7ffff6c282d0> ISRA.0_1 = ISRA.0;

A new SSA name "ISRA.0_2" is created, after setting definition statement for it
via:


  if (TREE_CODE (*tp) == SSA_NAME)
    {
      *tp = remap_ssa_name (*tp, id);
      *walk_subtrees = 0;
      if (is_lhs)
         SSA_NAME_DEF_STMT (*tp) = wi_p->stmt;
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      return NULL;
    }

Here, both "ISRA.0_1" and "ISRA.O_2" belong to same function, and point to same
def stmt.