[v4] ifcvt: Account for parallelism when costing noce sequences
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_simplebootstrap_build--master-aarch64-bootstrap |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_simplebootstrap_build--master-arm-bootstrap |
fail
|
Patch failed to apply
|
Commit Message
From: juewang <juewang@linux.alibaba.com>
seq_cost treats every instruction in a replacement sequence as serial.
This can overestimate simple register-only sequences on superscalar
targets and reject profitable if-conversion.
Build dependency levels for simple, unit-cost register operations.
Cost each level using the target issue rate. Fall back to the serial
cost for unsupported sequences, size optimization, and targets without
an issue rate hook.
Changes since v1:
v4: Format the multi-line conditional according to GNU coding style.
v3: Refactor instruction validation and dependency-level calculation into
separate helpers. Add comments explaining the intent of the parallel cost
model.
v2: Guard against calling targetm.sched.issue_rate when the hook is not
defined.
gcc/ChangeLog:
* ifcvt.cc (noce_parallel_cost_node): New.
(noce_parallel_costed_insn_p): New.
(noce_dependency_level): New.
(noce_parallel_seq_cost): New.
(default_noce_conversion_profitable_p): Use it.
---
gcc/ifcvt.cc | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 103 insertions(+), 1 deletion(-)
Comments
On 9/1/26 5:52 PM, wangjue wrote:
> seq_cost treats every instruction in a replacement sequence as serial.
> This can overestimate simple register-only sequences on superscalar
> targets and reject profitable if-conversion.
>
> Build dependency levels for simple, unit-cost register operations.
> Cost each level using the target issue rate. Fall back to the serial
> cost for unsupported sequences, size optimization, and targets without
> an issue rate hook.
>
> Changes since v1:
> v4: Format the multi-line conditional according to GNU coding style.
>
> v3: Refactor instruction validation and dependency-level calculation into
> separate helpers. Add comments explaining the intent of the parallel cost
> model.
>
> v2: Guard against calling targetm.sched.issue_rate when the hook is not
> defined.
>
> gcc/ChangeLog:
>
> * ifcvt.cc (noce_parallel_cost_node): New.
> (noce_parallel_costed_insn_p): New.
> (noce_dependency_level): New.
> (noce_parallel_seq_cost): New.
> (default_noce_conversion_profitable_p): Use it.
So the sh4 failure in my tester is almost certainly unrelated to this patch.
So I've pushed this version to the trunk. THanks for your patience.
For the future, please consider including testcases. There's numerous
folks that can help with that process. For this issue I would have
likely done a risc-v specific test because we can control things via
-mbranch-cost to verify that at a particular branch cost the 3
conditional move sequence if-converts.
jeff
> So the sh4 failure in my tester is almost certainly unrelated to this patch.
> So I've pushed this version to the trunk. THanks for your patience.
> For the future, please consider including testcases. There's numerous
> folks that can help with that process. For this issue I would have
> likely done a risc-v specific test because we can control things via
> -mbranch-cost to verify that at a particular branch cost the 3
> conditional move sequence if-converts.
Thanks for investigating the SH4 failure and pushing the patch to trunk.
And I’ll add the corresponding test case as soon as possible.
Best regards,
Wang Jue
On 9/2/26 6:24 PM, wangjue wrote:
>
>> So the sh4 failure in my tester is almost certainly unrelated to this patch.
>> So I've pushed this version to the trunk. THanks for your patience.
>> For the future, please consider including testcases. There's numerous
>> folks that can help with that process. For this issue I would have
>> likely done a risc-v specific test because we can control things via
>> -mbranch-cost to verify that at a particular branch cost the 3
>> conditional move sequence if-converts.
> Thanks for investigating the SH4 failure and pushing the patch to trunk.
> And I’ll add the corresponding test case as soon as possible.
Thanks.
An FYI. The compiler seems to have gotten dramatically slower in the
last 24hrs. We're going to need to bisect that and evaluate. I was
worried a bit about the compile-time performance of your patch, so
naturally to see a 2X increase in compile time after commiting is
concerning.
Jeff
On 9/2/26 8:10 PM, Jeff Law wrote:
>
>
> On 9/2/26 6:24 PM, wangjue wrote:
>>
>>> So the sh4 failure in my tester is almost certainly unrelated to
>>> this patch.
>>> So I've pushed this version to the trunk. THanks for your patience.
>>> For the future, please consider including testcases. There's numerous
>>> folks that can help with that process. For this issue I would have
>>> likely done a risc-v specific test because we can control things via
>>> -mbranch-cost to verify that at a particular branch cost the 3
>>> conditional move sequence if-converts.
>> Thanks for investigating the SH4 failure and pushing the patch to trunk.
>> And I’ll add the corresponding test case as soon as possible.
> Thanks.
>
> An FYI. The compiler seems to have gotten dramatically slower in the
> last 24hrs. We're going to need to bisect that and evaluate. I was
> worried a bit about the compile-time performance of your patch, so
> naturally to see a 2X increase in compile time after commiting is
> concerning.
Following up to myself. The slowdown isn't the compiler, it looks like
the sourceware/gcc machine has crawled to a standstill. So good news is
your patch is likely fine. Bad news nobody can likely get to the repos
right now.
jeff
This breaks m68k:
#0 fancy_abort (file=file@entry=0x222fa60 "../../gcc/gcc/config/m68k/m68k.cc",
line=line@entry=6597,
function=function@entry=0x23c5700 <m68k_sched_issue_rate()::__FUNCTION__> "m68k_sched_issue_rate") at ../../gcc/gcc/diagnostics/context.cc:1780
#1 0x00000000013d37be in m68k_sched_issue_rate ()
at ../../gcc/gcc/config/m68k/m68k.cc:6597
#2 0x0000000001e3073f in noce_parallel_seq_cost (speed_p=true,
seq=0x7ffff65ec000) at ../../gcc/gcc/ifcvt.cc:874
#3 default_noce_conversion_profitable_p (seq=0x7ffff65ec000,
if_info=0x7fffffffcea0) at ../../gcc/gcc/ifcvt.cc:921
#4 0x00000000006af899 in noce_try_store_flag_mask (if_info=0x7fffffffcea0)
at ../../gcc/gcc/ifcvt.cc:2298
#5 noce_process_if_block (if_info=if_info@entry=0x7fffffffcea0)
at ../../gcc/gcc/ifcvt.cc:5171
#6 0x0000000001e33cf0 in noce_find_if_block (
else_edge=<edge 0x7ffff7171498 (18 -> 20)>, pass=1,
then_edge=<edge 0x7ffff71714d0 (18 -> 19)>,
test_bb=<basic_block 0x7ffff6ce4180 (18)>) at ../../gcc/gcc/ifcvt.cc:5648
#7 find_if_header (pass=1, test_bb=<basic_block 0x7ffff6ce4180 (18)>)
at ../../gcc/gcc/ifcvt.cc:5853
#8 if_convert (after_combine=after_combine@entry=false)
at ../../gcc/gcc/ifcvt.cc:7000
#9 0x0000000001e34172 in rest_of_handle_if_conversion ()
at ../../gcc/gcc/ifcvt.cc:7065
You should never call targetm.sched.issue_rate if flag_schedule_insns is
zero.
> You should never call targetm.sched.issue_rate if flag_schedule_insns is
> zero.
I will add the appropriate guard and send an updated patch as soon as possible. Thanks for pointing this out
Best regards,
Wang Jue
Hi Jue,
I collected performance data with this patch on an x86 machine, with SPEC CPU2017/2026, there is no regression.
I have a slight concern: Why is only the new sequence costed in parallel, and not the original sequence? This leaves the comparison unfair.
Thanks,
Lili.
> -----Original Message-----
> From: wangjue <juewang@linux.alibaba.com>
> Sent: Thursday, September 3, 2026 8:24 AM
> To: Jeff Law <jeffrey.law@oss.qualcomm.com>; gcc-patches <gcc-
> patches@gcc.gnu.org>
> Cc: rdapp.gcc <rdapp.gcc@gmail.com>; richard.guenther
> <richard.guenther@gmail.com>; wangjue.wangjue
> <wangjue.wangjue@alibaba-inc.com>
> Subject: Re: [PATCH v4] ifcvt: Account for parallelism when costing noce
> sequences
>
>
>
> > So the sh4 failure in my tester is almost certainly unrelated to this patch.
>
> > So I've pushed this version to the trunk. THanks for your patience.
>
> > For the future, please consider including testcases. There's numerous
> > folks that can help with that process. For this issue I would have
> > likely done a risc-v specific test because we can control things via
> > -mbranch-cost to verify that at a particular branch cost the 3
> > conditional move sequence if-converts.
>
> Thanks for investigating the SH4 failure and pushing the patch to trunk.
> And I’ll add the corresponding test case as soon as possible.
>
>
> Best regards,
> Wang Jue
Hi Lili
> I collected performance data with this patch on an x86 machine, with SPEC CPU2017/2026, there is no regression.
Thanks for running the performance tests.
>I have a slight concern: Why is only the new sequence costed in parallel, and not the original sequence? This leaves the comparison unfair.
That sounds like a reasonable concern. I will experiment locally with changing
how the serial costs of the then and else blocks are calculated, and then rerun
the benchmarks to evaluate the performance impact.
Best regards,
Wang Jue
> -----Original Message-----
> From: wangjue <juewang@linux.alibaba.com>
> Sent: Thursday, September 3, 2026 7:23 PM
> To: Cui, Lili <lili.cui@intel.com>; Jeff Law <jeffrey.law@oss.qualcomm.com>;
> gcc-patches <gcc-patches@gcc.gnu.org>
> Cc: rdapp.gcc <rdapp.gcc@gmail.com>; wangjue.wangjue
> <wangjue.wangjue@alibaba-inc.com>
> Subject: Re: [PATCH v4] ifcvt: Account for parallelism when costing noce
> sequences
>
> Hi Lili
>
> > I collected performance data with this patch on an x86 machine, with SPEC
> CPU2017/2026, there is no regression.
> Thanks for running the performance tests.
>
>
> >I have a slight concern: Why is only the new sequence costed in parallel, and
> not the original sequence? This leaves the comparison unfair.
>
> That sounds like a reasonable concern. I will experiment locally with changing
> how the serial costs of the then and else blocks are calculated, and then rerun
> the benchmarks to evaluate the performance impact.
>
Sounds good, thanks!
BRs,
Lili
>
> Best regards,
> Wang Jue
>
>
@@ -803,6 +803,108 @@ noce_reversed_cond_code (struct noce_if_info *if_info)
return reversed_comparison_code (if_info->cond, if_info->jump);
}
+/* A register definition and its dependency level. */
+
+struct noce_parallel_cost_node
+{
+ rtx dest;
+ unsigned int level;
+};
+
+/* Return true if INSN is a unit-cost register SET supported by the parallel
+ cost model. Store the SET in *SET. */
+
+static bool
+noce_parallel_costed_insn_p (rtx_insn *insn, bool speed_p, rtx *set)
+{
+ *set = single_set (insn);
+ if (!*set)
+ return false;
+
+ rtx dest = SET_DEST (*set);
+ rtx src = SET_SRC (*set);
+
+ return (REG_P (dest)
+ && !contains_mem_rtx_p (src)
+ && !side_effects_p (src)
+ && !may_trap_p (src)
+ && set_rtx_cost (*set, speed_p) == COSTS_N_INSNS (1));
+}
+
+/* Return the dependency level for an instruction reading SRC. A use of a
+ register defined at level N requires level N + 1. */
+
+static unsigned int
+noce_dependency_level (const vec<noce_parallel_cost_node> &defs, rtx src)
+{
+ unsigned int level = 0;
+
+ for (unsigned int i = 0; i < defs.length (); ++i)
+ if (reg_overlap_mentioned_p (defs[i].dest, src))
+ level = MAX (level, defs[i].level + 1);
+
+ return level;
+}
+
+/* Estimate the cost of SEQ using the target issue rate. Unit-cost register
+ operations are grouped by RAW dependency level. The cost of each level is
+ its instruction count divided by the issue rate, rounded up. Fall back to
+ serial cost if any instruction cannot be modeled.
+
+ In an instrumented build of all SPEC CPU2017 Integer rate benchmarks
+ for RISC-V, the analysis handled 16,947 of 30,120 candidates (56.26%).
+ It reduced the cost for 15,024 of the handled candidates (88.65%).
+ The parallel-to-serial cost ratios for the handled candidates were:
+
+ 0.8 < ratio <= 1.0 11.54 percent
+ 0.6 < ratio <= 0.8 47.31 percent
+ 0.4 < ratio <= 0.6 40.24 percent
+ 0.2 < ratio <= 0.4 0.91 percent
+ 0.0 < ratio <= 0.2 0.00 percent. */
+
+static unsigned int
+noce_parallel_seq_cost (rtx_insn *seq, bool speed_p)
+{
+ unsigned int serial_cost = seq_cost (seq, speed_p);
+
+ if (!speed_p || !targetm.sched.issue_rate)
+ return serial_cost;
+
+ unsigned int issue_rate = MAX (targetm.sched.issue_rate (), 1);
+
+ /* Track prior definitions and instruction counts per dependency level. */
+ auto_vec<noce_parallel_cost_node> defs;
+ auto_vec<unsigned int> insns_per_level;
+
+ for (rtx_insn *insn = seq; insn; insn = NEXT_INSN (insn))
+ {
+ rtx set;
+
+ if (!NONDEBUG_INSN_P (insn))
+ continue;
+
+ if (!noce_parallel_costed_insn_p (insn, speed_p, &set))
+ return serial_cost;
+
+ unsigned int level = noce_dependency_level (defs, SET_SRC (set));
+
+ if (insns_per_level.length () <= level)
+ insns_per_level.safe_grow_cleared (level + 1, true);
+ ++insns_per_level[level];
+
+ noce_parallel_cost_node node = { SET_DEST (set), level };
+ defs.safe_push (node);
+ }
+
+ unsigned int parallel_cost = 0;
+ for (unsigned int i = 0; i < insns_per_level.length (); ++i)
+ parallel_cost += COSTS_N_INSNS (CEIL (insns_per_level[i], issue_rate));
+
+ /* set_rtx_cost and insn_cost can disagree, so cap the estimate at the
+ serial cost. */
+ return MIN (serial_cost, parallel_cost);
+}
+
/* Return true if SEQ is a good candidate as a replacement for the
if-convertible sequence described in IF_INFO.
This is the default implementation that targets can override
@@ -815,7 +917,7 @@ default_noce_conversion_profitable_p (rtx_insn *seq,
bool speed_p = if_info->speed_p;
/* Cost up the new sequence. */
- unsigned int cost = seq_cost (seq, speed_p);
+ unsigned int cost = noce_parallel_seq_cost (seq, speed_p);
if (cost <= if_info->original_cost)
return true;