[patch] Reuse non-gimple_reg variable for inlining
Hi,
when a call to a function is inlined and takes a parameter whose type is not
gimple_reg, a variable is created in the caller to hold a copy of the argument
passed in the call with the following comment:
/* We may produce non-gimple trees by adding NOPs or introduce
invalid sharing when operand is not really constant.
It is not big deal to prohibit constant propagation here as
we will constant propagate in DOM1 pass anyway. *
Of course the second sentence of the comment does not apply to non-gimple_reg
values, unless they get SRAed later, because we do not do constant propagation
for them. This for example prevents two identical calls to a pure function
from being merged in the attached Ada testcase.
Therefore the attached patch attempts to reuse a read-only or non-addressable
local DECL of the caller, the hitch being that expand_call_inline needs to be
prevented from creating a CLOBBER for the cases where it ends uo being reused.
Tested on x86-64/Linux, OK for the mainline?
2021-05-03 Eric Botcazou
* tree-inline.c (setup_one_parameter): Do not create a variable if the
value is either a read-only DECL or a non-addressable local variable.
Register the variable thus reused instead of creating a new one.
(expand_call_inline): Do not generate a CLOBBER for these variables.
2021-05-03 Eric Botcazou
* gnat.dg/opt94.adb: New test.
* gnat.dg/opt94_pkg.ads, opt94.adb: New helper.
--
Eric Botcazoudiff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 1dcb31c0267..a05093ab829 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -3460,16 +3460,18 @@ setup_one_parameter (copy_body_data *id, tree p, tree value, tree fn,
value. */
if (TREE_READONLY (p)
&& !TREE_ADDRESSABLE (p)
- && value && !TREE_SIDE_EFFECTS (value)
+ && value
+ && !TREE_SIDE_EFFECTS (value)
&& !def)
{
- /* We may produce non-gimple trees by adding NOPs or introduce
- invalid sharing when operand is not really constant.
- It is not big deal to prohibit constant propagation here as
- we will constant propagate in DOM1 pass anyway. */
- if (is_gimple_min_invariant (value)
- && useless_type_conversion_p (TREE_TYPE (p),
- TREE_TYPE (value))
+ /* We may produce non-gimple trees by adding NOPs or introduce invalid
+ sharing when the value is not constant or DECL. And we need to make
+ sure that it cannot be modified from another path in the callee. */
+ if ((is_gimple_min_invariant (value)
+ || (DECL_P (value) && TREE_READONLY (value))
+ || (auto_var_in_fn_p (value, id->src_fn)
+ && !TREE_ADDRESSABLE (value)))
+ && useless_type_conversion_p (TREE_TYPE (p), TREE_TYPE (value))
/* We have to be very careful about ADDR_EXPR. Make sure
the base variable isn't a local variable of the inlined
function, e.g., when doing recursive inlining, direct or
@@ -3478,6 +3480,13 @@ setup_one_parameter (copy_body_data *id, tree p, tree value, tree fn,
&& ! self_inlining_addr_expr (value, fn))
{
insert_decl_map (id, p, value);
+ /* Prevent expand_call_inline from creating a CLOBBER for it. */
+ if (VAR_P (value) & !is_gimple_reg (value))
+ {
+ if (!id->debug_map)
+ id->debug_map = new hash_map;
+ id->debug_map->put (value, value);
+ }
insert_debug_decl_map (id, p, var);
return insert_init_debug_bind (id, bb, var, value, NULL);
}
@@ -5129,7 +5138,10 @@ expand_call_inline (basic_block bb, gimple *stmt, copy_body_data *id,
if (!TREE_THIS_VOLATILE (p))
{
tree *varp = id->decl_map->get (p);
- if (varp && VAR_P (*varp) && !is_gimple_reg (*varp))
+ if (varp
+ && VAR_P (*varp)
+ && !is_gimple_reg (*varp)
+ && !(id->debug_map && id->debug_map->get (*varp)))
{
tree clobber = build_clobber (TREE_TYPE (*varp));
gimple *clobber_stmt;
-- { dg-do compile }
-- { dg-options "-O -gnatn -fdump-tree-optimized" }
with Opt94_Pkg; use Opt94_Pkg;
function Opt94 (S : String) return Integer is
A : constant String := Get;
begin
if Valid_Result (A) then
return Result (A);
else
return -1;
end if;
end;
-- { dg-final { scan-tree-dump-times "worker" 1 "optimized" } }
package body Opt94_Pkg is
function Worker (S : String) return Integer;
pragma Pure_Function (Worker);
function Valid_Result (S : String) return Boolean is
begin
return Worker (S) > 0;
end;
function Result (S : String) return Integer is
R : constant Integer := Worker (S);
begin
if R > 0 then
return R;
else
raise Program_Error;
end if;
end;
function Worker (S : String) return Integer is
begin
return Character'Pos (S (1));
end;
function Get return String is
begin
return "";
end;
end Opt94_Pkg;
package Opt94_Pkg is
function Valid_Result (S : String) return Boolean;
pragma Inline (Valid_Result);
function Result (S : String) retur
Re: [patch] Reuse non-gimple_reg variable for inlining
On Mon, May 3, 2021 at 11:02 AM Eric Botcazou wrote: > > Hi, > > when a call to a function is inlined and takes a parameter whose type is not > gimple_reg, a variable is created in the caller to hold a copy of the argument > passed in the call with the following comment: > > /* We may produce non-gimple trees by adding NOPs or introduce > invalid sharing when operand is not really constant. > It is not big deal to prohibit constant propagation here as > we will constant propagate in DOM1 pass anyway. * > > Of course the second sentence of the comment does not apply to non-gimple_reg > values, unless they get SRAed later, because we do not do constant propagation > for them. This for example prevents two identical calls to a pure function > from being merged in the attached Ada testcase. > > Therefore the attached patch attempts to reuse a read-only or non-addressable > local DECL of the caller, the hitch being that expand_call_inline needs to be > prevented from creating a CLOBBER for the cases where it ends uo being reused. > > Tested on x86-64/Linux, OK for the mainline? Hmm, instead of (ab-)using debug_map can we instead use sth like setting TREE_VISITED on the argument decl (not the value - it might be passed multiple tiimes)? IIRC TREE_VISITED state is undetermined thus we can clear it at the start of setup_one_parameter and set it when we want to avoid the clobber and then test for this later? In the end I'd even find using a new bitmap to record parameter decl UIDs cleaner ... (I'm not sure if we not end up doing tree walks that might clobber TREE_VISITED here). Otherwise looks OK. Thanks, Richard. > > 2021-05-03 Eric Botcazou > > * tree-inline.c (setup_one_parameter): Do not create a variable if the > value is either a read-only DECL or a non-addressable local variable. > Register the variable thus reused instead of creating a new one. > (expand_call_inline): Do not generate a CLOBBER for these variables. > > > 2021-05-03 Eric Botcazou > > * gnat.dg/opt94.adb: New test. > * gnat.dg/opt94_pkg.ads, opt94.adb: New helper. > > -- > Eric Botcazou
Re: [patch] Reuse non-gimple_reg variable for inlining
> Hmm, instead of (ab-)using debug_map can we instead use sth like setting
> TREE_VISITED on the argument decl (not the value - it might be passed
> multiple tiimes)? IIRC TREE_VISITED state is undetermined thus we can
> clear it at the start of setup_one_parameter and set it when we want to
> avoid the clobber and then test for this later?
>
> In the end I'd even find using a new bitmap to record parameter decl UIDs
> cleaner ... (I'm not sure if we not end up doing tree walks that might
> clobber TREE_VISITED here).
I tried TREE_VISITED and other similar kludges but this broke in weird ways so
reusing debug_map was probably the best of them. Less kludgy version attached
though, before switching to a dedicated bitmap indeed.
* tree-inline.c (insert_debug_decl_map): Insert unconditionally.
(copy_debug_stmt): Minor tweak.
(setup_one_parameter): Do not create a variable if the value is either
a read-only DECL or a non-addressable local variable in the caller.
(expand_call_inline): Do not generate a CLOBBER for these values.
--
Eric Botcazoudiff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 1dcb31c0267..57d3b6ff6de 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -152,21 +152,11 @@ insert_decl_map (copy_body_data *id, tree key, tree value)
id->decl_map->put (value, value);
}
-/* Insert a tree->tree mapping for ID. This is only used for
- variables. */
+/* Insert a tree->tree mapping for ID. This is only used for parameters. */
static void
insert_debug_decl_map (copy_body_data *id, tree key, tree value)
{
- if (!gimple_in_ssa_p (id->src_cfun))
-return;
-
- if (!opt_for_fn (id->dst_fn, flag_var_tracking_assignments))
-return;
-
- if (!target_for_debug_bind (key))
-return;
-
gcc_assert (TREE_CODE (key) == PARM_DECL);
gcc_assert (VAR_P (value));
@@ -3190,7 +3180,8 @@ copy_debug_stmt (gdebug *stmt, copy_body_data *id)
else
gcc_unreachable ();
- if (TREE_CODE (t) == PARM_DECL && id->debug_map
+ if (TREE_CODE (t) == PARM_DECL
+ && id->debug_map
&& (n = id->debug_map->get (t)))
{
gcc_assert (VAR_P (*n));
@@ -3460,16 +3451,18 @@ setup_one_parameter (copy_body_data *id, tree p, tree value, tree fn,
value. */
if (TREE_READONLY (p)
&& !TREE_ADDRESSABLE (p)
- && value && !TREE_SIDE_EFFECTS (value)
+ && value
+ && !TREE_SIDE_EFFECTS (value)
&& !def)
{
- /* We may produce non-gimple trees by adding NOPs or introduce
- invalid sharing when operand is not really constant.
- It is not big deal to prohibit constant propagation here as
- we will constant propagate in DOM1 pass anyway. */
- if (is_gimple_min_invariant (value)
- && useless_type_conversion_p (TREE_TYPE (p),
- TREE_TYPE (value))
+ /* We may produce non-gimple trees by adding NOPs or introduce invalid
+ sharing when the value is not constant or DECL. And we need to make
+ sure that it cannot be modified from another path in the callee. */
+ if ((is_gimple_min_invariant (value)
+ || (DECL_P (value) && TREE_READONLY (value))
+ || (auto_var_in_fn_p (value, id->src_fn)
+ && !TREE_ADDRESSABLE (value)))
+ && useless_type_conversion_p (TREE_TYPE (p), TREE_TYPE (value))
/* We have to be very careful about ADDR_EXPR. Make sure
the base variable isn't a local variable of the inlined
function, e.g., when doing recursive inlining, direct or
@@ -5128,8 +5121,13 @@ expand_call_inline (basic_block bb, gimple *stmt, copy_body_data *id,
for (tree p = DECL_ARGUMENTS (id->src_fn); p; p = DECL_CHAIN (p))
if (!TREE_THIS_VOLATILE (p))
{
+ /* The value associated with P is a local temporary only if
+ there is no value associated with P in the debug map. */
tree *varp = id->decl_map->get (p);
- if (varp && VAR_P (*varp) && !is_gimple_reg (*varp))
+ if (varp
+ && VAR_P (*varp)
+ && !is_gimple_reg (*varp)
+ && !(id->debug_map && id->debug_map->get (p)))
{
tree clobber = build_clobber (TREE_TYPE (*varp));
gimple *clobber_stmt;
Re: [patch] Reuse non-gimple_reg variable for inlining
On Mon, May 3, 2021 at 5:06 PM Eric Botcazou wrote: > > > Hmm, instead of (ab-)using debug_map can we instead use sth like setting > > TREE_VISITED on the argument decl (not the value - it might be passed > > multiple tiimes)? IIRC TREE_VISITED state is undetermined thus we can > > clear it at the start of setup_one_parameter and set it when we want to > > avoid the clobber and then test for this later? > > > > In the end I'd even find using a new bitmap to record parameter decl UIDs > > cleaner ... (I'm not sure if we not end up doing tree walks that might > > clobber TREE_VISITED here). > > I tried TREE_VISITED and other similar kludges but this broke in weird ways so > reusing debug_map was probably the best of them. Less kludgy version attached > though, before switching to a dedicated bitmap indeed. After simplifying insert_debug_decl_map this much, can you inline it at the single caller? OK with that change. Thanks, Richard. > > * tree-inline.c (insert_debug_decl_map): Insert unconditionally. > (copy_debug_stmt): Minor tweak. > (setup_one_parameter): Do not create a variable if the value is either > a read-only DECL or a non-addressable local variable in the caller. > (expand_call_inline): Do not generate a CLOBBER for these values. > > -- > Eric Botcazou
Re: [patch] Reuse non-gimple_reg variable for inlining
On Mon, May 03, 2021 at 10:04:20AM +0200, Eric Botcazou wrote: > Hi, > > when a call to a function is inlined and takes a parameter whose type is not > gimple_reg, a variable is created in the caller to hold a copy of the argument > passed in the call with the following comment: > > /* We may produce non-gimple trees by adding NOPs or introduce >invalid sharing when operand is not really constant. >It is not big deal to prohibit constant propagation here as >we will constant propagate in DOM1 pass anyway. * > > Of course the second sentence of the comment does not apply to non-gimple_reg > values, unless they get SRAed later, because we do not do constant propagation > for them. This for example prevents two identical calls to a pure function > from being merged in the attached Ada testcase. > > Therefore the attached patch attempts to reuse a read-only or non-addressable > local DECL of the caller, the hitch being that expand_call_inline needs to be > prevented from creating a CLOBBER for the cases where it ends uo being reused. I'm afraid the inliner would need to prove the to be inlined callee doesn't modify its own copy of the variable too, because if it modifies it (at least in C/C++ const can be cast away), then this introduces wrong-code, see PR100994 for details. > Tested on x86-64/Linux, OK for the mainline? > > > 2021-05-03 Eric Botcazou > > * tree-inline.c (setup_one_parameter): Do not create a variable if the > value is either a read-only DECL or a non-addressable local variable. > Register the variable thus reused instead of creating a new one. > (expand_call_inline): Do not generate a CLOBBER for these variables. > > > 2021-05-03 Eric Botcazou > > * gnat.dg/opt94.adb: New test. > * gnat.dg/opt94_pkg.ads, opt94.adb: New helper. Jakub
Re: [patch] Reuse non-gimple_reg variable for inlining
> I'm afraid the inliner would need to prove the to be inlined callee doesn't > modify its own copy of the variable too, because if it modifies it (at least > in C/C++ const can be cast away), then this introduces wrong-code, see > PR100994 for details. Then please remove the TREE_READONLY marker in C/C++ if this is a lie. -- Eric Botcazou
Re: [patch] Reuse non-gimple_reg variable for inlining
On Wed, 9 Jun 2021, Eric Botcazou wrote: > > I'm afraid the inliner would need to prove the to be inlined callee doesn't > > modify its own copy of the variable too, because if it modifies it (at least > > in C/C++ const can be cast away), then this introduces wrong-code, see > > PR100994 for details. > > Then please remove the TREE_READONLY marker in C/C++ if this is a lie. I agree the cases are invalid C/C++ - this isn't casting away const qualification of a pointed to type but of an object and storing into a readonly object at least invokes undefined behavior. It might even be allowed to diagnose it as error. Richard.
