* [PATCH 01/11] rtl-ssa: Support for inserting new insns
@ 2023-11-16 18:06 Alex Coplan
2023-11-21 11:51 ` Richard Sandiford
0 siblings, 1 reply; 3+ messages in thread
From: Alex Coplan @ 2023-11-16 18:06 UTC (permalink / raw)
To: gcc-patches; +Cc: Richard Sandiford, Kyrylo Tkachov
[-- Attachment #1: Type: text/plain, Size: 2942 bytes --]
N.B. this is just a rebased (but otherwise unchanged) version of the
same patch already posted here:
https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633348.html
this is the only unreviewed dependency from the previous series, so it
seemed easier just to re-post it (not least to appease the pre-commit
CI).
-- >8 --
The upcoming aarch64 load pair pass needs to form store pairs, and can
re-order stores over loads when alias analysis determines this is safe.
In the case that both mem defs have uses in the RTL-SSA IR, and both
stores require re-ordering over their uses, we represent that as
(tentative) deletion of the original store insns and creation of a new
insn, to prevent requiring repeated re-parenting of uses during the
pass. We then update all mem uses that require re-parenting in one go
at the end of the pass.
To support this, RTL-SSA needs to handle inserting new insns (rather
than just changing existing ones), so this patch adds support for that.
New insns (and new accesses) are temporaries, allocated above a temporary
obstack_watermark, such that the user can easily back out of a change without
awkward bookkeeping.
Bootstrapped/regtested as a series on aarch64-linux-gnu, OK for trunk?
gcc/ChangeLog:
* rtl-ssa/accesses.cc (function_info::create_set): New.
* rtl-ssa/accesses.h (access_info::is_temporary): New.
* rtl-ssa/changes.cc (move_insn): Handle new (temporary) insns.
(function_info::finalize_new_accesses): Handle new/temporary
user-created accesses.
(function_info::apply_changes_to_insn): Ensure m_is_temp flag
on new insns gets cleared.
(function_info::change_insns): Handle new/temporary insns.
(function_info::create_insn): New.
* rtl-ssa/changes.h (class insn_change): Make function_info a
friend class.
* rtl-ssa/functions.h (function_info): Declare new entry points:
create_set, create_insn. Declare new change_alloc helper.
* rtl-ssa/insns.cc (insn_info::print_full): Identify temporary insns in
dump.
* rtl-ssa/insns.h (insn_info): Add new m_is_temp flag and accompanying
is_temporary accessor.
* rtl-ssa/internals.inl (insn_info::insn_info): Initialize m_is_temp to
false.
* rtl-ssa/member-fns.inl (function_info::change_alloc): New.
* rtl-ssa/movement.h (restrict_movement_for_defs_ignoring): Add
handling for temporary defs.
---
gcc/rtl-ssa/accesses.cc | 10 ++++++
gcc/rtl-ssa/accesses.h | 4 +++
gcc/rtl-ssa/changes.cc | 74 +++++++++++++++++++++++++++++++-------
gcc/rtl-ssa/changes.h | 2 ++
gcc/rtl-ssa/functions.h | 14 ++++++++
gcc/rtl-ssa/insns.cc | 5 +++
gcc/rtl-ssa/insns.h | 7 +++-
gcc/rtl-ssa/internals.inl | 1 +
gcc/rtl-ssa/member-fns.inl | 12 +++++++
gcc/rtl-ssa/movement.h | 8 ++++-
10 files changed, 123 insertions(+), 14 deletions(-)
[-- Attachment #2: 0001-rtl-ssa-Support-for-inserting-new-insns.patch --]
[-- Type: text/x-patch, Size: 10546 bytes --]
diff --git a/gcc/rtl-ssa/accesses.cc b/gcc/rtl-ssa/accesses.cc
index 510545a8bad..76d70fd8bd3 100644
--- a/gcc/rtl-ssa/accesses.cc
+++ b/gcc/rtl-ssa/accesses.cc
@@ -1456,6 +1456,16 @@ function_info::make_uses_available (obstack_watermark &watermark,
return use_array (new_uses, num_uses);
}
+set_info *
+function_info::create_set (obstack_watermark &watermark,
+ insn_info *insn,
+ resource_info resource)
+{
+ auto set = change_alloc<set_info> (watermark, insn, resource);
+ set->m_is_temp = true;
+ return set;
+}
+
// Return true if ACCESS1 can represent ACCESS2 and if ACCESS2 can
// represent ACCESS1.
static bool
diff --git a/gcc/rtl-ssa/accesses.h b/gcc/rtl-ssa/accesses.h
index fce31d46717..7e7a90ece97 100644
--- a/gcc/rtl-ssa/accesses.h
+++ b/gcc/rtl-ssa/accesses.h
@@ -204,6 +204,10 @@ public:
// in the main instruction pattern.
bool only_occurs_in_notes () const { return m_only_occurs_in_notes; }
+ // Return true if this is a temporary access, e.g. one created for
+ // an insn that is about to be inserted.
+ bool is_temporary () const { return m_is_temp; }
+
protected:
access_info (resource_info, access_kind);
diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc
index aab532b9f26..da2a61d701a 100644
--- a/gcc/rtl-ssa/changes.cc
+++ b/gcc/rtl-ssa/changes.cc
@@ -394,14 +394,20 @@ move_insn (insn_change &change, insn_info *after)
// At the moment we don't support moving instructions between EBBs,
// but this would be worth adding if it's useful.
insn_info *insn = change.insn ();
- gcc_assert (after->ebb () == insn->ebb ());
+
bb_info *bb = after->bb ();
basic_block cfg_bb = bb->cfg_bb ();
- if (insn->bb () != bb)
- // Force DF to mark the old block as dirty.
- df_insn_delete (rtl);
- ::remove_insn (rtl);
+ if (!insn->is_temporary ())
+ {
+ gcc_assert (after->ebb () == insn->ebb ());
+
+ if (insn->bb () != bb)
+ // Force DF to mark the old block as dirty.
+ df_insn_delete (rtl);
+ ::remove_insn (rtl);
+ }
+
::add_insn_after (rtl, after_rtl, cfg_bb);
}
@@ -439,10 +445,15 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos)
gcc_assert (def);
if (def->m_is_temp)
{
- // At present, the only temporary instruction definitions we
- // create are clobbers, such as those added during recog.
- gcc_assert (is_a<clobber_info *> (def));
- def = allocate<clobber_info> (change.insn (), ref.regno);
+ if (is_a<clobber_info *> (def))
+ def = allocate<clobber_info> (change.insn (), ref.regno);
+ else if (is_a<set_info *> (def))
+ {
+ def->m_is_temp = false;
+ def = allocate<set_info> (change.insn (), def->resource ());
+ }
+ else
+ gcc_unreachable ();
}
else if (!def->m_has_been_superceded)
{
@@ -511,7 +522,9 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos)
unsigned int i = 0;
for (use_info *use : change.new_uses)
{
- if (!use->m_has_been_superceded)
+ if (use->m_is_temp)
+ use->m_has_been_superceded = true;
+ else if (!use->m_has_been_superceded)
{
use = allocate_temp<use_info> (insn, use->resource (), use->def ());
use->m_has_been_superceded = true;
@@ -645,6 +658,8 @@ function_info::apply_changes_to_insn (insn_change &change)
insn->set_accesses (builder.finish ().begin (), num_defs, num_uses);
}
+
+ insn->m_is_temp = false;
}
// Add a temporary placeholder instruction after AFTER.
@@ -677,7 +692,8 @@ function_info::change_insns (array_slice<insn_change *> changes)
if (!change->is_deletion ())
{
// Remove any notes that are no longer relevant.
- update_notes (change->rtl ());
+ if (!change->insn ()->m_is_temp)
+ update_notes (change->rtl ());
// Make sure that the placement of this instruction would still
// leave room for previous instructions.
@@ -686,6 +702,17 @@ function_info::change_insns (array_slice<insn_change *> changes)
// verify_insn_changes is supposed to make sure that this holds.
gcc_unreachable ();
min_insn = later_insn (min_insn, change->move_range.first);
+
+ if (change->insn ()->m_is_temp)
+ {
+ change->m_insn = allocate<insn_info> (change->insn ()->bb (),
+ change->rtl (),
+ change->insn_uid ());
+
+ // Set the flag again so subsequent logic is aware.
+ // It will be cleared later on.
+ change->m_insn->m_is_temp = true;
+ }
}
}
@@ -784,7 +811,8 @@ function_info::change_insns (array_slice<insn_change *> changes)
// Remove the placeholder first so that we have a wider range of
// program points when inserting INSN.
insn_info *after = placeholder->prev_any_insn ();
- remove_insn (insn);
+ if (!insn->is_temporary ())
+ remove_insn (insn);
remove_insn (placeholder);
insn->set_bb (after->bb ());
add_insn_after (insn, after);
@@ -1105,6 +1133,28 @@ function_info::perform_pending_updates ()
return changed_cfg;
}
+insn_info *
+function_info::create_insn (obstack_watermark &watermark,
+ rtx_code insn_code,
+ rtx pat)
+{
+ rtx_insn *rti = nullptr;
+
+ // TODO: extend, move in to emit-rtl.cc.
+ switch (insn_code)
+ {
+ case INSN:
+ rti = make_insn_raw (pat);
+ break;
+ default:
+ gcc_unreachable ();
+ }
+
+ auto insn = change_alloc<insn_info> (watermark, nullptr, rti, INSN_UID (rti));
+ insn->m_is_temp = true;
+ return insn;
+}
+
// Print a description of CHANGE to PP.
void
rtl_ssa::pp_insn_change (pretty_printer *pp, const insn_change &change)
diff --git a/gcc/rtl-ssa/changes.h b/gcc/rtl-ssa/changes.h
index d56e3a646e2..d91cf432afe 100644
--- a/gcc/rtl-ssa/changes.h
+++ b/gcc/rtl-ssa/changes.h
@@ -32,6 +32,8 @@ namespace rtl_ssa {
// something that we might do.
class insn_change
{
+ friend class function_info;
+
public:
enum delete_action { DELETE };
diff --git a/gcc/rtl-ssa/functions.h b/gcc/rtl-ssa/functions.h
index ecb40fdaf57..4ffd3fa44e2 100644
--- a/gcc/rtl-ssa/functions.h
+++ b/gcc/rtl-ssa/functions.h
@@ -68,6 +68,16 @@ public:
// Return the SSA information for CFG_BB.
bb_info *bb (basic_block cfg_bb) const { return m_bbs[cfg_bb->index]; }
+ // Create a temporary def.
+ set_info *create_set (obstack_watermark &watermark,
+ insn_info *insn,
+ resource_info resource);
+
+ // Create a temporary insn with code INSN_CODE and pattern PAT.
+ insn_info *create_insn (obstack_watermark &watermark,
+ rtx_code insn_code,
+ rtx pat);
+
// Return a list of all the instructions in the function, in reverse
// postorder. The list includes both real and artificial instructions.
//
@@ -195,6 +205,10 @@ public:
// Print the contents of the function to PP.
void print (pretty_printer *pp) const;
+ // Allocate an object of type T above the obstack watermark WM.
+ template<typename T, typename... Ts>
+ T *change_alloc (obstack_watermark &wm, Ts... args);
+
private:
class bb_phi_info;
class build_info;
diff --git a/gcc/rtl-ssa/insns.cc b/gcc/rtl-ssa/insns.cc
index 5fde3f2bb4b..2fa48e0dacd 100644
--- a/gcc/rtl-ssa/insns.cc
+++ b/gcc/rtl-ssa/insns.cc
@@ -192,6 +192,11 @@ insn_info::print_full (pretty_printer *pp) const
pp_newline_and_indent (pp, 0);
pp_string (pp, "has volatile refs");
}
+ if (m_is_temp)
+ {
+ pp_newline_and_indent (pp, 0);
+ pp_string (pp, "temporary");
+ }
}
pp_indentation (pp) -= 2;
}
diff --git a/gcc/rtl-ssa/insns.h b/gcc/rtl-ssa/insns.h
index a604fe295cd..6d0506706ad 100644
--- a/gcc/rtl-ssa/insns.h
+++ b/gcc/rtl-ssa/insns.h
@@ -306,6 +306,8 @@ public:
// Print a full description of the instruction.
void print_full (pretty_printer *) const;
+ bool is_temporary () const { return m_is_temp; }
+
private:
// The first-order way of representing the order between instructions
// is to assign "program points", with higher point numbers coming
@@ -414,8 +416,11 @@ private:
unsigned int m_has_pre_post_modify : 1;
unsigned int m_has_volatile_refs : 1;
+ // Indicates the insn is a temporary / new user-allocated insn.
+ unsigned int m_is_temp : 1;
+
// For future expansion.
- unsigned int m_spare : 27;
+ unsigned int m_spare : 26;
// The program point at which the instruction occurs.
//
diff --git a/gcc/rtl-ssa/internals.inl b/gcc/rtl-ssa/internals.inl
index e49297c12b3..907c4504352 100644
--- a/gcc/rtl-ssa/internals.inl
+++ b/gcc/rtl-ssa/internals.inl
@@ -415,6 +415,7 @@ inline insn_info::insn_info (bb_info *bb, rtx_insn *rtl, int cost_or_uid)
m_is_asm (false),
m_has_pre_post_modify (false),
m_has_volatile_refs (false),
+ m_is_temp (false),
m_spare (0),
m_point (0),
m_cost_or_uid (cost_or_uid),
diff --git a/gcc/rtl-ssa/member-fns.inl b/gcc/rtl-ssa/member-fns.inl
index ce2db045b78..b8940ca5566 100644
--- a/gcc/rtl-ssa/member-fns.inl
+++ b/gcc/rtl-ssa/member-fns.inl
@@ -962,4 +962,16 @@ function_info::add_regno_clobber (obstack_watermark &watermark,
return true;
}
+template<typename T, typename... Ts>
+inline T *
+function_info::change_alloc (obstack_watermark &wm, Ts... args)
+{
+ static_assert (std::is_trivially_destructible<T>::value,
+ "destructor won't be called");
+ static_assert (alignof (T) <= obstack_alignment,
+ "too much alignment required");
+ void *addr = XOBNEW (wm, T);
+ return new (addr) T (std::forward<Ts> (args)...);
+}
+
}
diff --git a/gcc/rtl-ssa/movement.h b/gcc/rtl-ssa/movement.h
index ec076db406f..41226dd3666 100644
--- a/gcc/rtl-ssa/movement.h
+++ b/gcc/rtl-ssa/movement.h
@@ -182,6 +182,11 @@ restrict_movement_for_defs_ignoring (insn_range_info &move_range,
{
for (def_info *def : defs)
{
+ // Skip fresh defs that are being inserted, as these shouldn't
+ // constrain movement.
+ if (def->is_temporary ())
+ continue;
+
// If the definition is a clobber, we can move it with respect
// to other clobbers.
//
@@ -247,7 +252,8 @@ restrict_movement_for_defs_ignoring (insn_range_info &move_range,
// Make sure that we don't move stores between basic blocks, since we
// don't have enough information to tell whether it's safe.
- if (def_info *def = memory_access (defs))
+ def_info *def = memory_access (defs);
+ if (def && !def->is_temporary ())
{
move_range = move_later_than (move_range, def->bb ()->head_insn ());
move_range = move_earlier_than (move_range, def->bb ()->end_insn ());
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 01/11] rtl-ssa: Support for inserting new insns
2023-11-16 18:06 [PATCH 01/11] rtl-ssa: Support for inserting new insns Alex Coplan
@ 2023-11-21 11:51 ` Richard Sandiford
2023-11-22 14:44 ` Alex Coplan
0 siblings, 1 reply; 3+ messages in thread
From: Richard Sandiford @ 2023-11-21 11:51 UTC (permalink / raw)
To: Alex Coplan; +Cc: gcc-patches, Kyrylo Tkachov
Alex Coplan <alex.coplan@arm.com> writes:
> N.B. this is just a rebased (but otherwise unchanged) version of the
> same patch already posted here:
>
> https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633348.html
>
> this is the only unreviewed dependency from the previous series, so it
> seemed easier just to re-post it (not least to appease the pre-commit
> CI).
>
> -- >8 --
>
> The upcoming aarch64 load pair pass needs to form store pairs, and can
> re-order stores over loads when alias analysis determines this is safe.
> In the case that both mem defs have uses in the RTL-SSA IR, and both
> stores require re-ordering over their uses, we represent that as
> (tentative) deletion of the original store insns and creation of a new
> insn, to prevent requiring repeated re-parenting of uses during the
> pass. We then update all mem uses that require re-parenting in one go
> at the end of the pass.
>
> To support this, RTL-SSA needs to handle inserting new insns (rather
> than just changing existing ones), so this patch adds support for that.
>
> New insns (and new accesses) are temporaries, allocated above a temporary
> obstack_watermark, such that the user can easily back out of a change without
> awkward bookkeeping.
>
> Bootstrapped/regtested as a series on aarch64-linux-gnu, OK for trunk?
>
> gcc/ChangeLog:
>
> * rtl-ssa/accesses.cc (function_info::create_set): New.
> * rtl-ssa/accesses.h (access_info::is_temporary): New.
> * rtl-ssa/changes.cc (move_insn): Handle new (temporary) insns.
> (function_info::finalize_new_accesses): Handle new/temporary
> user-created accesses.
> (function_info::apply_changes_to_insn): Ensure m_is_temp flag
> on new insns gets cleared.
> (function_info::change_insns): Handle new/temporary insns.
> (function_info::create_insn): New.
> * rtl-ssa/changes.h (class insn_change): Make function_info a
> friend class.
> * rtl-ssa/functions.h (function_info): Declare new entry points:
> create_set, create_insn. Declare new change_alloc helper.
> * rtl-ssa/insns.cc (insn_info::print_full): Identify temporary insns in
> dump.
> * rtl-ssa/insns.h (insn_info): Add new m_is_temp flag and accompanying
> is_temporary accessor.
> * rtl-ssa/internals.inl (insn_info::insn_info): Initialize m_is_temp to
> false.
> * rtl-ssa/member-fns.inl (function_info::change_alloc): New.
> * rtl-ssa/movement.h (restrict_movement_for_defs_ignoring): Add
> handling for temporary defs.
Looks good, but there were a couple of things I didn't understand:
> ---
> gcc/rtl-ssa/accesses.cc | 10 ++++++
> gcc/rtl-ssa/accesses.h | 4 +++
> gcc/rtl-ssa/changes.cc | 74 +++++++++++++++++++++++++++++++-------
> gcc/rtl-ssa/changes.h | 2 ++
> gcc/rtl-ssa/functions.h | 14 ++++++++
> gcc/rtl-ssa/insns.cc | 5 +++
> gcc/rtl-ssa/insns.h | 7 +++-
> gcc/rtl-ssa/internals.inl | 1 +
> gcc/rtl-ssa/member-fns.inl | 12 +++++++
> gcc/rtl-ssa/movement.h | 8 ++++-
> 10 files changed, 123 insertions(+), 14 deletions(-)
>
> diff --git a/gcc/rtl-ssa/accesses.cc b/gcc/rtl-ssa/accesses.cc
> index 510545a8bad..76d70fd8bd3 100644
> --- a/gcc/rtl-ssa/accesses.cc
> +++ b/gcc/rtl-ssa/accesses.cc
> @@ -1456,6 +1456,16 @@ function_info::make_uses_available (obstack_watermark &watermark,
> return use_array (new_uses, num_uses);
> }
>
> +set_info *
> +function_info::create_set (obstack_watermark &watermark,
> + insn_info *insn,
> + resource_info resource)
> +{
> + auto set = change_alloc<set_info> (watermark, insn, resource);
> + set->m_is_temp = true;
> + return set;
> +}
> +
> // Return true if ACCESS1 can represent ACCESS2 and if ACCESS2 can
> // represent ACCESS1.
> static bool
> diff --git a/gcc/rtl-ssa/accesses.h b/gcc/rtl-ssa/accesses.h
> index fce31d46717..7e7a90ece97 100644
> --- a/gcc/rtl-ssa/accesses.h
> +++ b/gcc/rtl-ssa/accesses.h
> @@ -204,6 +204,10 @@ public:
> // in the main instruction pattern.
> bool only_occurs_in_notes () const { return m_only_occurs_in_notes; }
>
> + // Return true if this is a temporary access, e.g. one created for
> + // an insn that is about to be inserted.
> + bool is_temporary () const { return m_is_temp; }
> +
> protected:
> access_info (resource_info, access_kind);
>
> diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc
> index aab532b9f26..da2a61d701a 100644
> --- a/gcc/rtl-ssa/changes.cc
> +++ b/gcc/rtl-ssa/changes.cc
> @@ -394,14 +394,20 @@ move_insn (insn_change &change, insn_info *after)
> // At the moment we don't support moving instructions between EBBs,
> // but this would be worth adding if it's useful.
> insn_info *insn = change.insn ();
> - gcc_assert (after->ebb () == insn->ebb ());
> +
> bb_info *bb = after->bb ();
> basic_block cfg_bb = bb->cfg_bb ();
>
> - if (insn->bb () != bb)
> - // Force DF to mark the old block as dirty.
> - df_insn_delete (rtl);
> - ::remove_insn (rtl);
> + if (!insn->is_temporary ())
> + {
> + gcc_assert (after->ebb () == insn->ebb ());
> +
> + if (insn->bb () != bb)
> + // Force DF to mark the old block as dirty.
> + df_insn_delete (rtl);
> + ::remove_insn (rtl);
> + }
> +
> ::add_insn_after (rtl, after_rtl, cfg_bb);
> }
>
> @@ -439,10 +445,15 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos)
> gcc_assert (def);
> if (def->m_is_temp)
> {
> - // At present, the only temporary instruction definitions we
> - // create are clobbers, such as those added during recog.
> - gcc_assert (is_a<clobber_info *> (def));
> - def = allocate<clobber_info> (change.insn (), ref.regno);
> + if (is_a<clobber_info *> (def))
> + def = allocate<clobber_info> (change.insn (), ref.regno);
> + else if (is_a<set_info *> (def))
> + {
> + def->m_is_temp = false;
> + def = allocate<set_info> (change.insn (), def->resource ());
Why is it necessary to clear is_temp on the old temporary set,
when it wasn't for the temporary clobber?
> + }
> + else
> + gcc_unreachable ();
> }
> else if (!def->m_has_been_superceded)
> {
> @@ -511,7 +522,9 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos)
> unsigned int i = 0;
> for (use_info *use : change.new_uses)
> {
> - if (!use->m_has_been_superceded)
> + if (use->m_is_temp)
> + use->m_has_been_superceded = true;
> + else if (!use->m_has_been_superceded)
Where do the temporary uses come from? It doesn't look like this patch
itself provided a new means of creating them.
Thanks,
Richard
> {
> use = allocate_temp<use_info> (insn, use->resource (), use->def ());
> use->m_has_been_superceded = true;
> @@ -645,6 +658,8 @@ function_info::apply_changes_to_insn (insn_change &change)
>
> insn->set_accesses (builder.finish ().begin (), num_defs, num_uses);
> }
> +
> + insn->m_is_temp = false;
> }
>
> // Add a temporary placeholder instruction after AFTER.
> @@ -677,7 +692,8 @@ function_info::change_insns (array_slice<insn_change *> changes)
> if (!change->is_deletion ())
> {
> // Remove any notes that are no longer relevant.
> - update_notes (change->rtl ());
> + if (!change->insn ()->m_is_temp)
> + update_notes (change->rtl ());
>
> // Make sure that the placement of this instruction would still
> // leave room for previous instructions.
> @@ -686,6 +702,17 @@ function_info::change_insns (array_slice<insn_change *> changes)
> // verify_insn_changes is supposed to make sure that this holds.
> gcc_unreachable ();
> min_insn = later_insn (min_insn, change->move_range.first);
> +
> + if (change->insn ()->m_is_temp)
> + {
> + change->m_insn = allocate<insn_info> (change->insn ()->bb (),
> + change->rtl (),
> + change->insn_uid ());
> +
> + // Set the flag again so subsequent logic is aware.
> + // It will be cleared later on.
> + change->m_insn->m_is_temp = true;
> + }
> }
> }
>
> @@ -784,7 +811,8 @@ function_info::change_insns (array_slice<insn_change *> changes)
> // Remove the placeholder first so that we have a wider range of
> // program points when inserting INSN.
> insn_info *after = placeholder->prev_any_insn ();
> - remove_insn (insn);
> + if (!insn->is_temporary ())
> + remove_insn (insn);
> remove_insn (placeholder);
> insn->set_bb (after->bb ());
> add_insn_after (insn, after);
> @@ -1105,6 +1133,28 @@ function_info::perform_pending_updates ()
> return changed_cfg;
> }
>
> +insn_info *
> +function_info::create_insn (obstack_watermark &watermark,
> + rtx_code insn_code,
> + rtx pat)
> +{
> + rtx_insn *rti = nullptr;
> +
> + // TODO: extend, move in to emit-rtl.cc.
> + switch (insn_code)
> + {
> + case INSN:
> + rti = make_insn_raw (pat);
> + break;
> + default:
> + gcc_unreachable ();
> + }
> +
> + auto insn = change_alloc<insn_info> (watermark, nullptr, rti, INSN_UID (rti));
> + insn->m_is_temp = true;
> + return insn;
> +}
> +
> // Print a description of CHANGE to PP.
> void
> rtl_ssa::pp_insn_change (pretty_printer *pp, const insn_change &change)
> diff --git a/gcc/rtl-ssa/changes.h b/gcc/rtl-ssa/changes.h
> index d56e3a646e2..d91cf432afe 100644
> --- a/gcc/rtl-ssa/changes.h
> +++ b/gcc/rtl-ssa/changes.h
> @@ -32,6 +32,8 @@ namespace rtl_ssa {
> // something that we might do.
> class insn_change
> {
> + friend class function_info;
> +
> public:
> enum delete_action { DELETE };
>
> diff --git a/gcc/rtl-ssa/functions.h b/gcc/rtl-ssa/functions.h
> index ecb40fdaf57..4ffd3fa44e2 100644
> --- a/gcc/rtl-ssa/functions.h
> +++ b/gcc/rtl-ssa/functions.h
> @@ -68,6 +68,16 @@ public:
> // Return the SSA information for CFG_BB.
> bb_info *bb (basic_block cfg_bb) const { return m_bbs[cfg_bb->index]; }
>
> + // Create a temporary def.
> + set_info *create_set (obstack_watermark &watermark,
> + insn_info *insn,
> + resource_info resource);
> +
> + // Create a temporary insn with code INSN_CODE and pattern PAT.
> + insn_info *create_insn (obstack_watermark &watermark,
> + rtx_code insn_code,
> + rtx pat);
> +
> // Return a list of all the instructions in the function, in reverse
> // postorder. The list includes both real and artificial instructions.
> //
> @@ -195,6 +205,10 @@ public:
> // Print the contents of the function to PP.
> void print (pretty_printer *pp) const;
>
> + // Allocate an object of type T above the obstack watermark WM.
> + template<typename T, typename... Ts>
> + T *change_alloc (obstack_watermark &wm, Ts... args);
> +
> private:
> class bb_phi_info;
> class build_info;
> diff --git a/gcc/rtl-ssa/insns.cc b/gcc/rtl-ssa/insns.cc
> index 5fde3f2bb4b..2fa48e0dacd 100644
> --- a/gcc/rtl-ssa/insns.cc
> +++ b/gcc/rtl-ssa/insns.cc
> @@ -192,6 +192,11 @@ insn_info::print_full (pretty_printer *pp) const
> pp_newline_and_indent (pp, 0);
> pp_string (pp, "has volatile refs");
> }
> + if (m_is_temp)
> + {
> + pp_newline_and_indent (pp, 0);
> + pp_string (pp, "temporary");
> + }
> }
> pp_indentation (pp) -= 2;
> }
> diff --git a/gcc/rtl-ssa/insns.h b/gcc/rtl-ssa/insns.h
> index a604fe295cd..6d0506706ad 100644
> --- a/gcc/rtl-ssa/insns.h
> +++ b/gcc/rtl-ssa/insns.h
> @@ -306,6 +306,8 @@ public:
> // Print a full description of the instruction.
> void print_full (pretty_printer *) const;
>
> + bool is_temporary () const { return m_is_temp; }
> +
> private:
> // The first-order way of representing the order between instructions
> // is to assign "program points", with higher point numbers coming
> @@ -414,8 +416,11 @@ private:
> unsigned int m_has_pre_post_modify : 1;
> unsigned int m_has_volatile_refs : 1;
>
> + // Indicates the insn is a temporary / new user-allocated insn.
> + unsigned int m_is_temp : 1;
> +
> // For future expansion.
> - unsigned int m_spare : 27;
> + unsigned int m_spare : 26;
>
> // The program point at which the instruction occurs.
> //
> diff --git a/gcc/rtl-ssa/internals.inl b/gcc/rtl-ssa/internals.inl
> index e49297c12b3..907c4504352 100644
> --- a/gcc/rtl-ssa/internals.inl
> +++ b/gcc/rtl-ssa/internals.inl
> @@ -415,6 +415,7 @@ inline insn_info::insn_info (bb_info *bb, rtx_insn *rtl, int cost_or_uid)
> m_is_asm (false),
> m_has_pre_post_modify (false),
> m_has_volatile_refs (false),
> + m_is_temp (false),
> m_spare (0),
> m_point (0),
> m_cost_or_uid (cost_or_uid),
> diff --git a/gcc/rtl-ssa/member-fns.inl b/gcc/rtl-ssa/member-fns.inl
> index ce2db045b78..b8940ca5566 100644
> --- a/gcc/rtl-ssa/member-fns.inl
> +++ b/gcc/rtl-ssa/member-fns.inl
> @@ -962,4 +962,16 @@ function_info::add_regno_clobber (obstack_watermark &watermark,
> return true;
> }
>
> +template<typename T, typename... Ts>
> +inline T *
> +function_info::change_alloc (obstack_watermark &wm, Ts... args)
> +{
> + static_assert (std::is_trivially_destructible<T>::value,
> + "destructor won't be called");
> + static_assert (alignof (T) <= obstack_alignment,
> + "too much alignment required");
> + void *addr = XOBNEW (wm, T);
> + return new (addr) T (std::forward<Ts> (args)...);
> +}
> +
> }
> diff --git a/gcc/rtl-ssa/movement.h b/gcc/rtl-ssa/movement.h
> index ec076db406f..41226dd3666 100644
> --- a/gcc/rtl-ssa/movement.h
> +++ b/gcc/rtl-ssa/movement.h
> @@ -182,6 +182,11 @@ restrict_movement_for_defs_ignoring (insn_range_info &move_range,
> {
> for (def_info *def : defs)
> {
> + // Skip fresh defs that are being inserted, as these shouldn't
> + // constrain movement.
> + if (def->is_temporary ())
> + continue;
> +
> // If the definition is a clobber, we can move it with respect
> // to other clobbers.
> //
> @@ -247,7 +252,8 @@ restrict_movement_for_defs_ignoring (insn_range_info &move_range,
>
> // Make sure that we don't move stores between basic blocks, since we
> // don't have enough information to tell whether it's safe.
> - if (def_info *def = memory_access (defs))
> + def_info *def = memory_access (defs);
> + if (def && !def->is_temporary ())
> {
> move_range = move_later_than (move_range, def->bb ()->head_insn ());
> move_range = move_earlier_than (move_range, def->bb ()->end_insn ());
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 01/11] rtl-ssa: Support for inserting new insns
2023-11-21 11:51 ` Richard Sandiford
@ 2023-11-22 14:44 ` Alex Coplan
0 siblings, 0 replies; 3+ messages in thread
From: Alex Coplan @ 2023-11-22 14:44 UTC (permalink / raw)
To: gcc-patches, Kyrylo Tkachov, richard.sandiford
On 21/11/2023 11:51, Richard Sandiford wrote:
> Alex Coplan <alex.coplan@arm.com> writes:
> > N.B. this is just a rebased (but otherwise unchanged) version of the
> > same patch already posted here:
> >
> > https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633348.html
> >
> > this is the only unreviewed dependency from the previous series, so it
> > seemed easier just to re-post it (not least to appease the pre-commit
> > CI).
> >
> > -- >8 --
> >
> > The upcoming aarch64 load pair pass needs to form store pairs, and can
> > re-order stores over loads when alias analysis determines this is safe.
> > In the case that both mem defs have uses in the RTL-SSA IR, and both
> > stores require re-ordering over their uses, we represent that as
> > (tentative) deletion of the original store insns and creation of a new
> > insn, to prevent requiring repeated re-parenting of uses during the
> > pass. We then update all mem uses that require re-parenting in one go
> > at the end of the pass.
> >
> > To support this, RTL-SSA needs to handle inserting new insns (rather
> > than just changing existing ones), so this patch adds support for that.
> >
> > New insns (and new accesses) are temporaries, allocated above a temporary
> > obstack_watermark, such that the user can easily back out of a change without
> > awkward bookkeeping.
> >
> > Bootstrapped/regtested as a series on aarch64-linux-gnu, OK for trunk?
> >
> > gcc/ChangeLog:
> >
> > * rtl-ssa/accesses.cc (function_info::create_set): New.
> > * rtl-ssa/accesses.h (access_info::is_temporary): New.
> > * rtl-ssa/changes.cc (move_insn): Handle new (temporary) insns.
> > (function_info::finalize_new_accesses): Handle new/temporary
> > user-created accesses.
> > (function_info::apply_changes_to_insn): Ensure m_is_temp flag
> > on new insns gets cleared.
> > (function_info::change_insns): Handle new/temporary insns.
> > (function_info::create_insn): New.
> > * rtl-ssa/changes.h (class insn_change): Make function_info a
> > friend class.
> > * rtl-ssa/functions.h (function_info): Declare new entry points:
> > create_set, create_insn. Declare new change_alloc helper.
> > * rtl-ssa/insns.cc (insn_info::print_full): Identify temporary insns in
> > dump.
> > * rtl-ssa/insns.h (insn_info): Add new m_is_temp flag and accompanying
> > is_temporary accessor.
> > * rtl-ssa/internals.inl (insn_info::insn_info): Initialize m_is_temp to
> > false.
> > * rtl-ssa/member-fns.inl (function_info::change_alloc): New.
> > * rtl-ssa/movement.h (restrict_movement_for_defs_ignoring): Add
> > handling for temporary defs.
>
> Looks good, but there were a couple of things I didn't understand:
Thanks for the review.
>
> > ---
> > gcc/rtl-ssa/accesses.cc | 10 ++++++
> > gcc/rtl-ssa/accesses.h | 4 +++
> > gcc/rtl-ssa/changes.cc | 74 +++++++++++++++++++++++++++++++-------
> > gcc/rtl-ssa/changes.h | 2 ++
> > gcc/rtl-ssa/functions.h | 14 ++++++++
> > gcc/rtl-ssa/insns.cc | 5 +++
> > gcc/rtl-ssa/insns.h | 7 +++-
> > gcc/rtl-ssa/internals.inl | 1 +
> > gcc/rtl-ssa/member-fns.inl | 12 +++++++
> > gcc/rtl-ssa/movement.h | 8 ++++-
> > 10 files changed, 123 insertions(+), 14 deletions(-)
> >
> > diff --git a/gcc/rtl-ssa/accesses.cc b/gcc/rtl-ssa/accesses.cc
> > index 510545a8bad..76d70fd8bd3 100644
> > --- a/gcc/rtl-ssa/accesses.cc
> > +++ b/gcc/rtl-ssa/accesses.cc
> > @@ -1456,6 +1456,16 @@ function_info::make_uses_available (obstack_watermark &watermark,
> > return use_array (new_uses, num_uses);
> > }
> >
> > +set_info *
> > +function_info::create_set (obstack_watermark &watermark,
> > + insn_info *insn,
> > + resource_info resource)
> > +{
> > + auto set = change_alloc<set_info> (watermark, insn, resource);
> > + set->m_is_temp = true;
> > + return set;
> > +}
> > +
> > // Return true if ACCESS1 can represent ACCESS2 and if ACCESS2 can
> > // represent ACCESS1.
> > static bool
> > diff --git a/gcc/rtl-ssa/accesses.h b/gcc/rtl-ssa/accesses.h
> > index fce31d46717..7e7a90ece97 100644
> > --- a/gcc/rtl-ssa/accesses.h
> > +++ b/gcc/rtl-ssa/accesses.h
> > @@ -204,6 +204,10 @@ public:
> > // in the main instruction pattern.
> > bool only_occurs_in_notes () const { return m_only_occurs_in_notes; }
> >
> > + // Return true if this is a temporary access, e.g. one created for
> > + // an insn that is about to be inserted.
> > + bool is_temporary () const { return m_is_temp; }
> > +
> > protected:
> > access_info (resource_info, access_kind);
> >
> > diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc
> > index aab532b9f26..da2a61d701a 100644
> > --- a/gcc/rtl-ssa/changes.cc
> > +++ b/gcc/rtl-ssa/changes.cc
> > @@ -394,14 +394,20 @@ move_insn (insn_change &change, insn_info *after)
> > // At the moment we don't support moving instructions between EBBs,
> > // but this would be worth adding if it's useful.
> > insn_info *insn = change.insn ();
> > - gcc_assert (after->ebb () == insn->ebb ());
> > +
> > bb_info *bb = after->bb ();
> > basic_block cfg_bb = bb->cfg_bb ();
> >
> > - if (insn->bb () != bb)
> > - // Force DF to mark the old block as dirty.
> > - df_insn_delete (rtl);
> > - ::remove_insn (rtl);
> > + if (!insn->is_temporary ())
> > + {
> > + gcc_assert (after->ebb () == insn->ebb ());
> > +
> > + if (insn->bb () != bb)
> > + // Force DF to mark the old block as dirty.
> > + df_insn_delete (rtl);
> > + ::remove_insn (rtl);
> > + }
> > +
> > ::add_insn_after (rtl, after_rtl, cfg_bb);
> > }
> >
> > @@ -439,10 +445,15 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos)
> > gcc_assert (def);
> > if (def->m_is_temp)
> > {
> > - // At present, the only temporary instruction definitions we
> > - // create are clobbers, such as those added during recog.
> > - gcc_assert (is_a<clobber_info *> (def));
> > - def = allocate<clobber_info> (change.insn (), ref.regno);
> > + if (is_a<clobber_info *> (def))
> > + def = allocate<clobber_info> (change.insn (), ref.regno);
> > + else if (is_a<set_info *> (def))
> > + {
> > + def->m_is_temp = false;
> > + def = allocate<set_info> (change.insn (), def->resource ());
>
> Why is it necessary to clear is_temp on the old temporary set,
> when it wasn't for the temporary clobber?
In the case of a store pair insn (in the parallel form), I think we will
see two writes of memory in properties.refs (). If we're inserting a
new stp insn with a newly-created set of memory, then we must ensure
that we only try to add one def of memory to m_temp_defs, or we will get
an invalid access array.
If we don't clear m_is_temp on the old def, then we will allocate and push two
defs of memory in this case, which is clearly wrong. I don't think the same
situation can happen with clobbers. AIUI, non-memory resources can only have at
most one write in properties.refs ().
Having said that, and having looked again at the code more closely, I
don't think clearing the flag alone is the right solution, as the second
time we process a memory write in this case, we will call
record_reference against the old (temporary) def, and thus any changes
made by record_reference will be lost.
I think instead we should replace the temporary def with the
newly-allocated (permanent) def in change.new_defs, and then push a
pointer to this new def to m_temp_defs.
So perhaps we could use find_access_index instead of find_access, and
in the case that we find a temporary def, do something like:
def = allocate<set_info> (change.insn (), def->resource ());
change.new_defs[def_index] = def;
WDYT?
>
> > + }
> > + else
> > + gcc_unreachable ();
> > }
> > else if (!def->m_has_been_superceded)
> > {
> > @@ -511,7 +522,9 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos)
> > unsigned int i = 0;
> > for (use_info *use : change.new_uses)
> > {
> > - if (!use->m_has_been_superceded)
> > + if (use->m_is_temp)
> > + use->m_has_been_superceded = true;
> > + else if (!use->m_has_been_superceded)
>
> Where do the temporary uses come from? It doesn't look like this patch
> itself provided a new means of creating them.
Ah, sorry, I think this is a hold-over from a previous iteration of the
patch where we did provide an entry point for creating new uses. I
dropped it in favour of teaching RTL-SSA to infer uses of mem
(g:505f1202e3a1a1aecd0df10d0f1620df6fea4ab5).
I'll drop this in the next iteration of the patch.
Thanks,
Alex
>
> Thanks,
> Richard
>
> > {
> > use = allocate_temp<use_info> (insn, use->resource (), use->def ());
> > use->m_has_been_superceded = true;
> > @@ -645,6 +658,8 @@ function_info::apply_changes_to_insn (insn_change &change)
> >
> > insn->set_accesses (builder.finish ().begin (), num_defs, num_uses);
> > }
> > +
> > + insn->m_is_temp = false;
> > }
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-11-22 14:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-16 18:06 [PATCH 01/11] rtl-ssa: Support for inserting new insns Alex Coplan
2023-11-21 11:51 ` Richard Sandiford
2023-11-22 14:44 ` Alex Coplan
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).