On Mon, Oct 20, 2025 at 5:56 AM Andrew Pinski
<[email protected]> wrote:
>
> In expand, a bunch of places chose between 2 sequences based on if the cost
> is less. Currently this is done manually and does NOT always print out
> which one is selected and their costs.
>
> This adds a 2 classes which is designed to record the sequences and choose
> one of
> them based on their cost.
> This is a RFC rather than a full patch as I don't like the class names I came
> up with.
> It transform one example to use the new class to show how it will be used.
> There is at least one place where I was going to choose between more than 2
> sequences, that
> is why I used a vec here (though with a reserved space for 3 elements). I
> suspect this would
> be used more if this in is in place as it should make it easier add a new
> sequence for choosing.
>
> Note I know I misspelled choose as chose. I will correct before submitting
> the full path; but I
> wanted to get people thoughts on the overall idea before fixing it up.
>
> The 2 questions I have is:
> 1) is this a good idea to handle the choice between 2 (or more) sequences done
> this way or should the code kept like it was before?
> 2) better names for the 2 structs/classes?
>
> gcc/ChangeLog:
>
> * expr.cc (sequence_with_cost): New struct.
> (chose_sequence): New struct.
> (expand_expr_divmod): Use sequence_with_cost/chose_sequence
> for choosing between unsigned/signed div/mod sequence.
>
> Signed-off-by: Andrew Pinski <[email protected]>
> ---
> gcc/expr.cc | 85 +++++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 63 insertions(+), 22 deletions(-)
>
> diff --git a/gcc/expr.cc b/gcc/expr.cc
> index 4a699101bb5..1a4397959f6 100644
> --- a/gcc/expr.cc
> +++ b/gcc/expr.cc
> @@ -67,6 +67,58 @@ along with GCC; see the file COPYING3. If not see
> #include "flags.h"
> #include "internal-fn.h"
>
> +namespace {
> + struct sequence_with_cost
> + {
> + rtx_insn *m_seq;
> + rtx m_target;
> + const char *m_name;
> + unsigned m_cost;
> + unsigned m_alt_cost;
> + unsigned m_index;
> + static int cmp(const void *av, const void *bv)
> + {
> + const sequence_with_cost *a = (const sequence_with_cost*)av;
> + const sequence_with_cost *b = (const sequence_with_cost*)bv;
> + if (a->m_cost == b->m_cost)
> + {
> + if (a->m_alt_cost == b->m_alt_cost)
> + return a->m_index - b->m_index;
> + return a->m_alt_cost - b->m_alt_cost;
> + }
> + return a->m_cost - b->m_cost;
> + }
> + };
I think this struct is over-engineering.
> + struct chose_sequence
> + {
> + auto_vec<sequence_with_cost, 3> m_seqs;
Just have auto_vec<rtx_insn *, 3>?
> + void push_seq (rtx_insn *seq, rtx t, const char *name)
> + {
> + bool speed_p = optimize_insn_for_speed_p ();
> + unsigned cost = seq_cost (seq, speed_p);
> + unsigned alt_cost = seq_cost (seq, !speed_p);
> + m_seqs.safe_push({seq, t, name, cost, alt_cost, m_seqs.length()});
> + }
> + const sequence_with_cost &chose (const char *name)
> + {
> + gcc_assert (m_seqs.length () != 0);
> + if (m_seqs.length () == 1)
> + return m_seqs[0];
> + m_seqs.qsort (sequence_with_cost::cmp);
And you don't need to sort to find the best sequence, you just
do one linear scan and compute the best candidate against
the other. You then can compute the "tie" !speed_p cost on-demand.
'seq_alts' is my bikeshedding contribution.
Richard.
> + if (dump_file && (dump_flags & TDF_DETAILS))
> + {
> + fprintf (dump_file, ";; sorted costs for %s:\n", name);
> + for (auto &e : m_seqs)
> + {
> + fprintf (dump_file, ";; %u, %s: ", e.m_index, e.m_name);
> + fprintf (dump_file, "%u (alt: %u)\n", e.m_cost, e.m_alt_cost);
> + }
> + }
> + return m_seqs[0];
> + }
> + };
> +}
> +
>
> /* If this is nonzero, we do not bother generating VOLATILE
> around volatile memory references, and we are willing to
> @@ -9765,7 +9817,6 @@ expand_expr_divmod (tree_code code, machine_mode mode,
> tree treeop0,
> /* If both arguments are known to be positive when interpreted
> as signed, we can expand it as both signed and unsigned
> division or modulo. Choose the cheaper sequence in that case. */
> - bool speed_p = optimize_insn_for_speed_p ();
> do_pending_stack_adjust ();
> start_sequence ();
> rtx uns_ret = expand_divmod (mod_p, code, mode, op0, op1, target, 1);
> @@ -9773,31 +9824,21 @@ expand_expr_divmod (tree_code code, machine_mode
> mode, tree treeop0,
> start_sequence ();
> rtx sgn_ret = expand_divmod (mod_p, code, mode, op0, op1, target, 0);
> rtx_insn *sgn_insns = end_sequence ();
> - unsigned uns_cost = seq_cost (uns_insns, speed_p);
> - unsigned sgn_cost = seq_cost (sgn_insns, speed_p);
> - bool was_tie = false;
> -
> - /* If costs are the same then use as tie breaker the other other
> - factor. */
> - if (uns_cost == sgn_cost)
> + chose_sequence seqs;
> + /* Prefer based on the original signedness seq for tie breaker. */
> + if (unsignedp)
> {
> - uns_cost = seq_cost (uns_insns, !speed_p);
> - sgn_cost = seq_cost (sgn_insns, !speed_p);
> - was_tie = true;
> + seqs.push_seq (uns_insns, uns_ret, "unsigned");
> + seqs.push_seq (sgn_insns, sgn_ret, "signed");
> }
> -
> - if (dump_file && (dump_flags & TDF_DETAILS))
> - fprintf (dump_file, ";; positive division:%s unsigned cost: %u; "
> - "signed cost: %u\n",
> - was_tie ? " (needed tie breaker)" : "", uns_cost, sgn_cost);
> -
> - if (uns_cost < sgn_cost || (uns_cost == sgn_cost && unsignedp))
> + else
> {
> - emit_insn (uns_insns);
> - return uns_ret;
> + seqs.push_seq (sgn_insns, sgn_ret, "signed");
> + seqs.push_seq (uns_insns, uns_ret, "unsigned");
> }
> - emit_insn (sgn_insns);
> - return sgn_ret;
> + const auto &seq_with_cost = seqs.chose ("positive division");
> + emit_insn (seq_with_cost.m_seq);
> + return seq_with_cost.m_target;
> }
> return expand_divmod (mod_p, code, mode, op0, op1, target, unsignedp);
> }
> --
> 2.43.0
>
