[Bug target/109610] New: [14 regression] gcc.target/powerpc/dform-3.c fails after

[Bug target/109610] New: [14 regression] gcc.target/powerpc/dform-3.c fails after

[seurer at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109610

            Bug ID: 109610
           Summary: [14 regression] gcc.target/powerpc/dform-3.c fails
                    after
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: seurer at gcc dot gnu.org
  Target Milestone: ---

g:0368d169492017cfab5622d38b15be94154d458c, r14-172-g0368d169492017

make  -k check-gcc RUNTESTFLAGS="powerpc.exp=gcc.target/powerpc/dform-3.c"

FAIL: gcc.target/powerpc/dform-3.c scan-assembler-not mfvsrd 
FAIL: gcc.target/powerpc/dform-3.c scan-assembler-not mfvsrld 
# of expected passes            7
# of unexpected failures        2


commit 0368d169492017cfab5622d38b15be94154d458c (HEAD, refs/bisect/bad)
Author: liuhongt <hongtao.liu@intel.com>
Date:   Wed Feb 8 12:42:27 2023 +0800

    Use NO_REGS in cost calculation when the preferred register class are not
known yet.

[Bug target/109610] [14 regression] gcc.target/powerpc/dform-3.c fails after r14-172-g0368d169492017

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109610

--- Comment #1 from Hongtao.liu <crazylht at gmail dot com> ---
Mine, I'll take a look.

[Bug target/109610] [14 regression] gcc.target/powerpc/dform-3.c fails after r14-172-g0368d169492017

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109610

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |14.0
           Keywords|                            |testsuite-fail

[Bug target/109610] [14 regression] gcc.target/powerpc/dform-3.c fails after r14-172-g0368d169492017

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109610

--- Comment #2 from Hongtao.liu <crazylht at gmail dot com> ---
testcase:

#ifndef TYPE
#define TYPE vector double
#endif

struct foo {
  TYPE a, b, c, d;
};

/* Make sure we don't use direct moves to get stuff into GPR registers.  */
void
gpr (struct foo *p)
{
  TYPE x = p->c;

  __asm__ (" # reg = %0" : "+r" (x));

  p->b = x;
}


Here's dump in ira.

-------------
a2(r117,l0) costs: BASE_REGS:12000,12000 GENERAL_REGS:12000,12000
FLOAT_REGS:0,0 ALTIVEC_REGS:0,0 VSX_REGS:0,0 GEN_OR_FLOAT_REGS:12000,12000
GEN_OR_VSX_REGS:12000,12000 MEM:0,0
....
(insn 6 3 13 2 (set (reg/v:V2DF 117 [ x ])
        (mem:V2DF (plus:DI (reg/v/f:DI 118 [ p ])
                (const_int 32 [0x20])) [1 p_2(D)->c+0 S16 A128]))
"testsuite/gcc.target/powerpc/dform-3.c":23:8 1148 {vsx_movv2df_64bit}
     (nil))
(insn 13 6 8 2 (set (reg:V2DF 119 [ x ])
        (reg/v:V2DF 117 [ x ])) "testsuite/gcc.target/powerpc/dform-3.c":25:3
-1
     (nil))
(insn 8 13 9 2 (parallel [
            (set (reg:V2DF 119 [ x ])
                (asm_operands:V2DF (" # reg = %0") ("=r") 0 [
                        (reg:V2DF 119 [ x ])
                    ]
                     [
                        (asm_input:V2DF ("0")
testsuite/gcc.target/powerpc/dform-3.c:25)
                    ]
                     [] testsuite/gcc.target/powerpc/dform-3.c:25))
            (clobber (reg:SI 98 ca))
        ]) "testsuite/gcc.target/powerpc/dform-3.c":25:3 -1
---------------


And after my commit, RA take best scenario when preferred reg_class is unkown,
which make cost of MEM:0,0 of r117 same as VSX_REGS:0,0, and allocate r117 as
VSX_REGS, which create an extra move and failed the testcase.

[Bug target/109610] [14 regression] gcc.target/powerpc/dform-3.c fails after r14-172-g0368d169492017

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109610

--- Comment #3 from Hongtao.liu <crazylht at gmail dot com> ---

> 
> And after my commit, RA take best scenario when preferred reg_class is
> unkown, which make cost of MEM:0,0 of r117 same as VSX_REGS:0,0, and
> allocate r117 as VSX_REGS, which create an extra move and failed the
> testcase.

The extra move normally can be eliminated by reload if both src and dest have
same reg_class, but here src is VSX_REGS and dest is GENERAL_REGS.

Maybe we should add a little bit extra cost to best scenario to make RA handle
such redundancy but not relies on pass_reload for that.

@@ -1580,7 +1580,7 @@ scan_one_insn (rtx_insn *insn)
       int num = COST_INDEX (REGNO (reg));

       COSTS (costs, num)->mem_cost
-       -= ira_memory_move_cost[GET_MODE (reg)][cl][1] * frequency;
+       -= (ira_memory_move_cost[GET_MODE (reg)][cl][1] + 1) * frequency;

1 unit cost is aligned with what's did in recog_reg_class

                  /* If the alternative actually allows memory, make
                     things a bit cheaper since we won't need an extra
                     insn to load it.  */
                  pp->mem_cost
                    = ((out_p ? ira_memory_move_cost[mode][op_class][0] : 0)
                       + (in_p ? ira_memory_move_cost[mode][op_class][1] : 0)
                       - allows_mem[i]) * frequency;

[Bug target/109610] [14 regression] gcc.target/powerpc/dform-3.c fails after r14-172-g0368d169492017

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109610

--- Comment #4 from Hongtao.liu <crazylht at gmail dot com> ---
> 1 unit cost is aligned with what's did in recog_reg_class
> 
> 		  /* If the alternative actually allows memory, make
> 		     things a bit cheaper since we won't need an extra
> 		     insn to load it.  */
> 		  pp->mem_cost
> 		    = ((out_p ? ira_memory_move_cost[mode][op_class][0] : 0)
> 		       + (in_p ? ira_memory_move_cost[mode][op_class][1] : 0)
> 		       - allows_mem[i]) * frequency;

But it regressed other cases, .i.e

gcc/testsuite/gcc.target/i386/pr18041-1.c

[Bug target/109610] [14 regression] gcc.target/powerpc/dform-3.c fails after r14-172-g0368d169492017

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109610

--- Comment #5 from Hongtao.liu <crazylht at gmail dot com> ---
One solution is add an peephole for handle such redudancy.

+;; Peephole to catch memory loads to VSX_REG and then moves to GENERAL_REGS.
+(define_peephole2
+  [(set (match_operand:VSX_M 0 "vsx_register_operand")
+       (match_operand:VSX_M 1 "memory_operand"))
+   (set (match_operand:VSX_M 2 "int_reg_operand")
+       (match_dup 0))]
+  "TARGET_POWERPC64 && VECTOR_MEM_VSX_P (<MODE>mode)
+  && peep2_reg_dead_p (2, operands[0])"
+  [(set (match_dup 2) (match_dup 1))])


If powerpc maintainer doesn't like this way, another alternative is add a
target hook in RA to still use GENEREAL_REGS for other targets, but use NO_REGS
only for x86.

[Bug target/109610] [14 regression] gcc.target/powerpc/dform-3.c fails after r14-172-g0368d169492017

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109610

rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rsandifo at gcc dot gnu.org

--- Comment #6 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
Please don't do the peephole thing!  This seems like a
target-independent problem.

The costs for r117 look odd.  Why is the cost of GENERAL_REGS so high
when the use (before the introduction of insn 13) explicitly requires
GENERAL_REGS?

Given those costs, the behaviour after the patch looks correct
(on the basis of the information its working with, I mean,
even though it's not the desired effect).

[Bug target/109610] [14 regression] gcc.target/powerpc/dform-3.c fails after r14-172-g0368d169492017

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109610

--- Comment #7 from Hongtao.liu <crazylht at gmail dot com> ---
(In reply to rsandifo@gcc.gnu.org from comment #6)
> Please don't do the peephole thing!  This seems like a
> target-independent problem.
> 
> The costs for r117 look odd.  Why is the cost of GENERAL_REGS so high
> when the use (before the introduction of insn 13) explicitly requires
> GENERAL_REGS?
> 
> Given those costs, the behaviour after the patch looks correct
> (on the basis of the information its working with, I mean,
> even though it's not the desired effect).

(define_insn "vsx_mov<mode>_64bit"
  [(set (match_operand:VSX_M 0 "nonimmediate_operand"
               "=ZwO,      wa,        wa,        r,         we,        ?wQ,
                ?&r,       ??r,       ??Y,       <??r>,     wa,        v,
                wa,        wa,
                ?wa,       v,         <??r>,     wZ,        v")

        (match_operand:VSX_M 1 "input_operand" 
               "wa,        ZwO,       wa,        we,        r,         r,
                wQ,        Y,         r,         r,         wE,        jwM,
                eQ,        eP,
                ?jwM,      W,         <nW>,      v,         wZ"))]

  "TARGET_POWERPC64 && VECTOR_MEM_VSX_P (<MODE>mode)
   && (register_operand (operands[0], <MODE>mode) 
       || register_operand (operands[1], <MODE>mode))"
{
  return rs6000_output_move_128bit (operands);
}

Because the backend pattern explicitly disparage the alternative (<??r>, r),
(??r, Y) which moves from GENERAL_REGS/MEM to GENERAL_REGS. And in cost
calculation, RA will add extra 2 for each '?', that's why cost of GENERAL_REGS
is so high. If manually remove ?? from ??r, then the cost for GENERAL_REGS will
become 0, then RA can allocate r117 as GENERAL_REGS, the extra move can be
eliminated by pass_reload.  

----------cost after removing ?? from ??r--------------
a2(r117,l0) costs: BASE_REGS:0,0 GENERAL_REGS:0,0 FLOAT_REGS:0,0
ALTIVEC_REGS:0,0 VSX_REGS:0,0 GEN_OR_FLOAT_REGS:12000,12000
GEN_OR_VSX_REGS:12000,12000 MEM:0,0
-----------end-----------------------

So it looks like an target dependent issue, the backend dislike allocating
GENERAL_REGS for V2DFmode move, but inline asm explicitly want it to be in
GENERAL_REGS.

[Bug target/109610] [14 regression] gcc.target/powerpc/dform-3.c fails after r14-172-g0368d169492017

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109610

Peter Bergner <bergner at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |meissner at gcc dot gnu.org,
                   |                            |segher at gcc dot gnu.org

--- Comment #8 from Peter Bergner <bergner at gcc dot gnu.org> ---
Adding Mike and Segher for their input.

[Bug target/109610] [14 regression] gcc.target/powerpc/dform-3.c fails after r14-172-g0368d169492017

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109610

--- Comment #9 from Hongtao.liu <crazylht at gmail dot com> ---
A patch is posted at
https://gcc.gnu.org/pipermail/gcc-patches/2023-May/617431.html

[Bug target/109610] [14 regression] gcc.target/powerpc/dform-3.c fails after r14-172-g0368d169492017

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109610

--- Comment #10 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Hongtao.liu from comment #5)
> One solution is add an peephole for handle such redudancy.

Not okay.

> If powerpc maintainer doesn't like this way, another alternative is add a
> target hook in RA to still use GENEREAL_REGS for other targets, but use
> NO_REGS only for x86.

Also not okay.  Please solve the fundamental problem in cost estimation you
created, don't let all targets try to fix it in different ways :-(

[Bug target/109610] [14 regression] gcc.target/powerpc/dform-3.c fails after r14-172-g0368d169492017

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109610

--- Comment #11 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Hongtao.liu from comment #5)
> One solution is add an peephole for handle such redudancy.

Not okay.

> If powerpc maintainer doesn't like this way, another alternative is add a
> target hook in RA to still use GENEREAL_REGS for other targets, but use
> NO_REGS only for x86.

Also not okay.  Please solve the fundamental problem in cost estimation you
created, don't let all targets try to fix it in different ways :-(

[Bug target/109610] [14 regression] gcc.target/powerpc/dform-3.c fails after r14-172-g0368d169492017

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109610

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |linkw at gcc dot gnu.org

--- Comment #12 from Kewen Lin <linkw at gcc dot gnu.org> ---
By looking into this failure, I found something tricky related to the cost
adjustment for this kind of destination pseudo:

  /* If this insn loads a parameter from its stack slot, then it
     represents a savings, rather than a cost, if the parameter is
     stored in memory.  Record this fact.

     Similarly if we're loading other constants from memory (constant
     pool, TOC references, small data areas, etc) and this is the only
     assignment to the destination pseudo.

Hongtao already has one fix for this regression (thanks!), but IMHO this tricky
thing might affect any future adjustment on this particular cost, so I posted
my findings here:

This above adjustment can happen on the destination pseudo of a load insn with
one equiv note, but the equiv note can be created later by update_equiv_regs
during ira (it's the case this failure here), it means this memory cost
reduction isn't considered in the first pass of best class determination, whose
resulted pref used for the second pass of best class determination can be not
good enough (not the best). In the 2nd pass, when scanning insn 13
r119:V2DF=r117:V2DF, cost calculation would respect the pref VSX_REGS for r117,
penalizes alternative whose operand 1 have operand class NO_REGS (r117), but at
this time the pref isn't the best as the pref VSX_REGS determination doesn't
consider the memory cost reduction, so I think we should give it a chance to
re-consider NO_REGS and not to penalize that corresponding alternative. Not
sure this kind of re-consideration (forgiving if it's NO_REGS) is worthy or
not, actually it's independent of this regression issue, maybe I can give it a
shot with some SPEC2017 runs.

[Bug target/109610] [14 regression] gcc.target/powerpc/dform-3.c fails after r14-172-g0368d169492017

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109610

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by hongtao Liu <liuhongt@gcc.gnu.org>:

https://gcc.gnu.org/g:4fb66b2329319e9b47e89200d613b6f741a114fc

commit r14-1252-g4fb66b2329319e9b47e89200d613b6f741a114fc
Author: liuhongt <hongtao.liu@intel.com>
Date:   Tue May 16 10:36:16 2023 +0800

    Only use NO_REGS in cost calculation when !hard_regno_mode_ok for
GENERAL_REGS and mode.

    r14-172-g0368d169492017 replaces GENERAL_REGS with NO_REGS in cost
    calculation when the preferred register class are not known yet.
    It regressed powerpc PR109610 and PR109858, it looks too aggressive to use
    NO_REGS when mode can be allocated with GENERAL_REGS.
    The patch takes a step back, still use GENERAL_REGS when
    hard_regno_mode_ok for mode and GENERAL_REGS, otherwise uses NO_REGS.

    gcc/ChangeLog:

            PR target/109610
            PR target/109858
            * ira-costs.cc (scan_one_insn): Only use NO_REGS in cost
            calculation when !hard_regno_mode_ok for GENERAL_REGS and
            mode, otherwise still use GENERAL_REGS.

[Bug target/109610] [14 regression] gcc.target/powerpc/dform-3.c fails after r14-172-g0368d169492017

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109610

--- Comment #14 from Hongtao.liu <crazylht at gmail dot com> ---
Fixed for GCC14.

[Bug target/109610] [14 regression] gcc.target/powerpc/dform-3.c fails after r14-172-g0368d169492017

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109610

--- Comment #15 from Peter Bergner <bergner at gcc dot gnu.org> ---
Bill, the original patch went in before GCC 13 was released and I only see a
trunk/GCC 14 fix.  Is dform-3.c still broken when testing GCC 13?  Ie, do we
need a backport of the fix?

[Bug target/109610] [14 regression] gcc.target/powerpc/dform-3.c fails after r14-172-g0368d169492017

[seurer at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109610

seurer at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

--- Comment #16 from seurer at gcc dot gnu.org ---
I do not see the problem currently occurring in gcc 13.