Thanks for your reply. We looked into ivopts in more detail to better understand it, and we found an alternative solution (https://gcc.gnu.org/pipermail/gcc-patches/2026-May/717400.html) for this issue.
Also, we agree with you that in some point we should get rid of the 'complexity' tie-breaker completely. Best regards, Radosav On 4/30/26 14:30, Richard Biener wrote: > CAUTION: This email originated from outside of the organization. Do not click > links or open attachments unless you recognize the sender and know the > content is safe. > > > On Tue, Mar 10, 2026 at 4:47 PM Radosav Krunic > <[email protected]> wrote: >> >> On 3/4/26 15:14, Richard Biener wrote: >>> CAUTION: This email originated from outside of the organization. Do not >>> click links or open attachments unless you recognize the sender and know >>> the content is safe. >>> >>> >>> On Wed, Mar 4, 2026 at 2:49 PM Radosav Krunic >>> <[email protected]> wrote: >>>> This patch addresses a regression introduced in r8-568-gf9f69dd651b2f1 >>>> (i.e., refactoring ivopts pass) for target mips-r6-linux-gnu. It >>>> corrects the complexity calculation in ivopts. The fix involves >>>> complexity computation reordering and proper invariant variables >>>> handling in address expressions. These changes align with the approach >>>> used before r8-568-gf9f69dd651b2f1 (e.g., in parent commit c2b64ce). The >>>> improved complexity calculations ensure better candidate selection and >>>> reduced code size, particularly for RISC CPUs. >>>> >>>> Signed-off-by: Radosav Krunic <[email protected]> >>>> Signed-off-by: Aleksandar Rakic <[email protected]> >>>> Signed-off-by: Jovan Dmitrovic <[email protected]> >>> Please state how you tested this patch. >> It was tested on an x86_64-pc-linux-gnu host. A cross-compiler toolchain >> was built and installed for the target mips64-r6-linux-gnu, and the GCC >> regression tests were run under QEMU. The command used was: >> >> make -k -j$(nproc) check-gcc \ >> RUNTESTFLAGS="--target_board=mips-sim-mti64_64/-march=mips64r6/-mabi=64 \ >> SIM='$EMUPATH -L $SYSROOT'" >> >> Here, $EMUPATH is the path to QEMU, and $SYSROOT is the path to the >> sysroot directory, which is a subdirectory of the installation prefix >> directory. Testing was performed with GCC at commit a4954130d43 >> ("c++: define __cpp_pack_indexing [PR113798]"). >> The tests completed successfully with no new regressions. > So trying to reproduce, the testcase scans around .L6 but the > important loop nest > is L5/L4, and code generation changes from > > .align 3 > .L5: > daddu $3,$10,$4 > move $2,$11 > lwc1 $f2,0($3) > .align 3 > .L4: > daddu $3,$8,$2 > lwc1 $f0,0($2) > daddu $3,$3,$4 > daddiu $2,$2,4 > lwc1 $f1,0($3) > maddf.s $f0,$f2,$f1 > bne $7,$2,.L4 > swc1 $f0,-4($2) > > addiu $9,$9,1 > bne $6,$9,.L5 > daddu $4,$4,$12 > > to > > .align 3 > .L5: > daddu $2,$9,$8 > daddu $3,$4,$8 > lwc1 $f2,-4($2) > move $2,$18 > .align 3 > .L4: > lwc1 $f1,0($3) > daddiu $2,$2,4 > lwc1 $f0,-4($2) > daddiu $3,$3,4 > maddf.s $f0,$f2,$f1 > bne $7,$2,.L4 > swc1 $f0,-4($2) > > addiu $17,$17,1 > bne $6,$17,.L5 > daddu $8,$8,$10 > > we're making the inner loop one increment shorter for > getting that into the outer loop plus one reg-reg move. If I understand > mips assembly correctly. > > I let Claude summarize the patch and there's two pieces that should > be probably considered seprately. First is that the attempts to > handle a constant offset and to move a fixed address to symbol part > are considered even when neither index nor scaled index addressing > works. Second is to increment complexity when there's not simple > invariant parts (but that's not complexity of the address, invariant > parts are costed separately). > > I'm somewhat sympathetic to the first piece. But it's the complexity > part that makes the difference and on its own it creates better result > that with the rest: > > .align 3 > .L5: > lwc1 $f2,0($9) > daddu $3,$18,$9 > move $2,$19 > .align 3 > .L4: > lwc1 $f1,0($3) > daddiu $2,$2,4 > lwc1 $f0,-4($2) > daddiu $3,$3,4 > maddf.s $f0,$f2,$f1 > bne $8,$2,.L4 > swc1 $f0,-4($2) > > addiu $17,$17,1 > bne $6,$17,.L5 > daddu $9,$9,$10 > > so I'm not convinced you understand what's going on or have assessed > the impact of the change (I do not, of course, I know nothing of mips > and unfortunately not much about IVOPTs either). > > I'd rather get rid of the 'complexity' tie-breaker completely. > > Richard. > > >>>> gcc/ChangeLog: >>>> >>>> * tree-ssa-loop-ivopts.cc (get_address_cost): Fixed >>>> complexity calculation. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> * gcc.target/mips/bug_tree-optimization_109429.c: New test. >>>> --- >>>> .../mips/bug_tree-optimization_109429.c | 24 +++++++++ >>>> gcc/tree-ssa-loop-ivopts.cc | 54 +++++++++---------- >>>> 2 files changed, 50 insertions(+), 28 deletions(-) >>>> create mode 100644 >>>> gcc/testsuite/gcc.target/mips/bug_tree-optimization_109429.c >>>> >>>> diff --git a/gcc/testsuite/gcc.target/mips/bug_tree-optimization_109429.c >>>> b/gcc/testsuite/gcc.target/mips/bug_tree-optimization_109429.c >>>> new file mode 100644 >>>> index 00000000000..96a62278fcd >>>> --- /dev/null >>>> +++ b/gcc/testsuite/gcc.target/mips/bug_tree-optimization_109429.c >>>> @@ -0,0 +1,24 @@ >>>> +/* { dg-do compile} */ >>>> +/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */ >>>> +/* { dg-options "-march=mips64r6 -mabi=64" } */ >>>> + >>>> +static void daxpy (float *vector1, float *vector2, int n, float fp_const) >>>> +{ >>>> + for (int i = 0; i < n; ++i) >>>> + vector1[i] += fp_const * vector2[i]; >>>> +} >>>> + >>>> +void dgefa (float *vector, int m, int n, int l) >>>> +{ >>>> + for (int i = 0; i < n - 1; ++i) >>>> + { >>>> + for (int j = i + 1; j < n; ++j) >>>> + { >>>> + float t = vector[m * j + l]; >>>> + daxpy (&vector[m * i + i + 1], >>>> + &vector[m * j + i + 1], n - (i + 1), t); >>>> + } >>>> + } >>>> +} >>>> + >>>> +/* { dg-final { scan-assembler >>>> "\\.L6:\\n\\tlwc1\t\\\$f1,0\\\(\\\$7\\\)\\n\tdaddiu\t\\\$2,\\\$2,4\\n\tlwc1\t\\\$f0,-4\\\(\\\$2\\\)" >>>> } } */ >>>> diff --git a/gcc/tree-ssa-loop-ivopts.cc b/gcc/tree-ssa-loop-ivopts.cc >>>> index 6ecf5bef7b4..0df365671ef 100644 >>>> --- a/gcc/tree-ssa-loop-ivopts.cc >>>> +++ b/gcc/tree-ssa-loop-ivopts.cc >>>> @@ -4695,38 +4695,35 @@ get_address_cost (struct ivopts_data *data, struct >>>> iv_use *use, >>>> if (!ok_with_ratio_p) >>>> parts.step = NULL_TREE; >>>> } >>>> - if (ok_with_ratio_p || ok_without_ratio_p) >>>> + if (!(ok_with_ratio_p || ok_without_ratio_p)) >>>> + parts.index = NULL_TREE; >>>> + if (maybe_ne (aff_inv->offset, 0)) >>>> { >>>> - if (maybe_ne (aff_inv->offset, 0)) >>>> - { >>>> - parts.offset = wide_int_to_tree (sizetype, aff_inv->offset); >>>> - /* Addressing mode "base + index [<< scale] + offset". */ >>>> - if (!valid_mem_ref_p (mem_mode, as, &parts, code)) >>>> - parts.offset = NULL_TREE; >>>> - else >>>> - aff_inv->offset = 0; >>>> - } >>>> + parts.offset = wide_int_to_tree (sizetype, aff_inv->offset); >>>> + /* Addressing mode "base[+ index[<< scale]] + offset". */ >>>> + if (!valid_mem_ref_p (mem_mode, as, &parts, code)) >>>> + parts.offset = NULL_TREE; >>>> + else >>>> + aff_inv->offset = 0; >>>> + } >>>> >>>> - move_fixed_address_to_symbol (&parts, aff_inv); >>>> - /* Base is fixed address and is moved to symbol part. */ >>>> - if (parts.symbol != NULL_TREE && aff_combination_zero_p >>>> (aff_inv)) >>>> - parts.base = NULL_TREE; >>>> + move_fixed_address_to_symbol (&parts, aff_inv); >>>> + /* Base is fixed address and is moved to symbol part. */ >>>> + if (parts.symbol != NULL_TREE && aff_combination_zero_p (aff_inv)) >>>> + parts.base = NULL_TREE; >>>> >>>> - /* Addressing mode "symbol + base + index [<< scale] [+ >>>> offset]". */ >>>> - if (parts.symbol != NULL_TREE >>>> - && !valid_mem_ref_p (mem_mode, as, &parts, code)) >>>> - { >>>> - aff_combination_add_elt (aff_inv, parts.symbol, 1); >>>> - parts.symbol = NULL_TREE; >>>> - /* Reset SIMPLE_INV since symbol address needs to be computed >>>> - outside of address expression in this case. */ >>>> - simple_inv = false; >>>> - /* Symbol part is moved back to base part, it can't be NULL. >>>> */ >>>> - parts.base = integer_one_node; >>>> - } >>>> + /* Addressing mode "symbol + base[+ index[<< scale]] [+ offset]". >>>> */ >>>> + if (parts.symbol != NULL_TREE >>>> + && !valid_mem_ref_p (mem_mode, as, &parts, code)) >>>> + { >>>> + aff_combination_add_elt (aff_inv, parts.symbol, 1); >>>> + parts.symbol = NULL_TREE; >>>> + /* Reset SIMPLE_INV since symbol address needs to be computed >>>> + outside of address expression in this case. */ >>>> + simple_inv = false; >>>> + /* Symbol part is moved back to base part, it can't be NULL. */ >>>> + parts.base = integer_one_node; >>>> } >>>> - else >>>> - parts.index = NULL_TREE; >>>> } >>>> else >>>> { >>>> @@ -4787,6 +4784,7 @@ get_address_cost (struct ivopts_data *data, struct >>>> iv_use *use, >>>> neutralize such effects. */ >>>> cost.cost = adjust_setup_cost (data, cost.cost, true); >>>> cost.scratch = cost.cost; >>>> + cost.complexity += 1; >>>> } >>>> >>>> cost += var_cost; >>>> -- >>>> 2.43.0
