diff mbox series

Fix wrong code out of NRV + RSO + inlining (take 2)

Message ID 5613124.rdbgypaU67@fomalhaut
State New
Headers
Series Fix wrong code out of NRV + RSO + inlining (take 2) |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Test passed

Commit Message

Eric Botcazou Oct. 1, 2024, 10 a.m. UTC 
  Hi,

the attached Ada testcase compiled with -O -flto exhibits a wrong code issue 
when the 3 optimizations NRV + RSO + inlining are applied to the same call: if 
the LHS of the call is marked write-only before inlining, then it will keep 
the mark after inlining although it may be read in GIMPLE from that point on.

The proposed fix is to apply the removal of the store that would have been 
applied by execute_fixup_cfg if the call was not inlined:

	  /* For calls we can simply remove LHS when it is known
	     to be write-only.  */
	  if (is_gimple_call (stmt)
	      && gimple_get_lhs (stmt))
	    {
	      tree lhs = get_base_address (gimple_get_lhs (stmt));

	      if (VAR_P (lhs)
		  && (TREE_STATIC (lhs) || DECL_EXTERNAL (lhs))
		  && varpool_node::get (lhs)->writeonly)
		{
		  gimple_call_set_lhs (stmt, NULL);
		  update_stmt (stmt);
	          todo |= TODO_update_ssa | TODO_cleanup_cfg;
		}
	    }

right before inlining, which will prevent the problematic references to the 
LHS from being generated during inlining.

Tested on x86-64/Linux, OK for the mainline?


2024-10-01  Eric Botcazou  <ebotcazou@adacore.com>

	* tree-inline.cc (expand_call_inline): Remove the store to the
	return slot if it is a global variable that is only written to.


2024-10-01  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/lto28.adb: New test.
	* gnat.dg/lto28_pkg1.ads: New helper.
	* gnat.dg/lto28_pkg2.ads: Likewise.
	* gnat.dg/lto28_pkg2.adb: Likewise.
	* gnat.dg/lto28_pkg3.ads: Likewise.

Comments

Richard Biener Oct. 1, 2024, 12:30 p.m. UTC | #1 
On Tue, Oct 1, 2024 at 12:01 PM Eric Botcazou <botcazou@adacore.com> wrote:
>
> Hi,
>
> the attached Ada testcase compiled with -O -flto exhibits a wrong code issue
> when the 3 optimizations NRV + RSO + inlining are applied to the same call: if
> the LHS of the call is marked write-only before inlining, then it will keep
> the mark after inlining although it may be read in GIMPLE from that point on.
>
> The proposed fix is to apply the removal of the store that would have been
> applied by execute_fixup_cfg if the call was not inlined:
>
>           /* For calls we can simply remove LHS when it is known
>              to be write-only.  */
>           if (is_gimple_call (stmt)
>               && gimple_get_lhs (stmt))
>             {
>               tree lhs = get_base_address (gimple_get_lhs (stmt));
>
>               if (VAR_P (lhs)
>                   && (TREE_STATIC (lhs) || DECL_EXTERNAL (lhs))
>                   && varpool_node::get (lhs)->writeonly)
>                 {
>                   gimple_call_set_lhs (stmt, NULL);
>                   update_stmt (stmt);
>                   todo |= TODO_update_ssa | TODO_cleanup_cfg;
>                 }
>             }
>
> right before inlining, which will prevent the problematic references to the
> LHS from being generated during inlining.
>
> Tested on x86-64/Linux, OK for the mainline?

Nice solution - OK.

Thanks,
Richard.

>
> 2024-10-01  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * tree-inline.cc (expand_call_inline): Remove the store to the
>         return slot if it is a global variable that is only written to.
>
>
> 2024-10-01  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gnat.dg/lto28.adb: New test.
>         * gnat.dg/lto28_pkg1.ads: New helper.
>         * gnat.dg/lto28_pkg2.ads: Likewise.
>         * gnat.dg/lto28_pkg2.adb: Likewise.
>         * gnat.dg/lto28_pkg3.ads: Likewise.
>
> --
> Eric Botcazou
diff mbox series

Patch

diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc
index f31a34ac410..8d43b033319 100644
--- a/gcc/tree-inline.cc
+++ b/gcc/tree-inline.cc
@@ -5130,9 +5130,23 @@  expand_call_inline (basic_block bb, gimple *stmt, copy_body_data *id,
       if (DECL_P (modify_dest))
 	suppress_warning (modify_dest, OPT_Wuninitialized);
 
+      /* If we have a return slot, we can assign it the result directly,
+	 except in the case where it is a global variable that is only
+	 written to because, the callee being permitted to read or take
+	 the address of its DECL_RESULT, this would invalidate the flag
+	 on the global variable; instead we preventively remove the store,
+	 which would have happened later if the call was not inlined.  */
       if (gimple_call_return_slot_opt_p (call_stmt))
 	{
-	  return_slot = modify_dest;
+	  tree base = get_base_address (modify_dest);
+
+	  if (VAR_P (base)
+	      && (TREE_STATIC (base) || DECL_EXTERNAL (base))
+	      && varpool_node::get (base)->writeonly)
+	    return_slot = NULL;
+	  else
+	    return_slot = modify_dest;
+
 	  modify_dest = NULL;
 	}
     }