diff mbox series

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

Message ID 20230517065702.2935000-1-hongtao.liu@intel.com
State Accepted
Headers
Series Only use NO_REGS in cost calculation when !hard_regno_mode_ok for GENERAL_REGS and mode. |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

liuhongt May 17, 2023, 6:57 a.m. UTC 
  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.
Kewen confirmed the patch fixed PR109858, I vefiried it also fixed PR109610.

Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,}.
No big performance impact for SPEC2017 on icelake server.
Ok for trunk?

gcc/ChangeLog:

	* 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.
---
 gcc/ira-costs.cc | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Jeff Law May 19, 2023, 9:31 p.m. UTC | #1 
On 5/17/23 00:57, liuhongt via Gcc-patches wrote:
> 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.
> Kewen confirmed the patch fixed PR109858, I vefiried it also fixed PR109610.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,}.
> No big performance impact for SPEC2017 on icelake server.
> Ok for trunk?
> 
> gcc/ChangeLog:
> 
> 	* 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.
BTW, Vlad is on PTO right now.  I'm sure he'll handle this after he 
returns and starts digging out of all the stuff that's piled up.

jeff
Vladimir Makarov May 25, 2023, 2:29 p.m. UTC | #2 
On 5/17/23 02:57, liuhongt wrote:
> 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.
> Kewen confirmed the patch fixed PR109858, I vefiried it also fixed PR109610.
>
> Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,}.
> No big performance impact for SPEC2017 on icelake server.
> Ok for trunk?
>
> gcc/ChangeLog:
>
> 	* 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.

Thank you for the patch.  It looks good for me.  It is ok to commit it 
into the trunk.
Segher Boessenkool May 25, 2023, 3:37 p.m. UTC | #3 
On Thu, May 25, 2023 at 10:29:47AM -0400, Vladimir Makarov wrote:
> 
> On 5/17/23 02:57, liuhongt wrote:
> >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.
> >Kewen confirmed the patch fixed PR109858, I vefiried it also fixed 
> >PR109610.
> >
> >Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,}.
> >No big performance impact for SPEC2017 on icelake server.
> >Ok for trunk?
> >
> >gcc/ChangeLog:
> >
> >	* 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.
> 
> Thank you for the patch.  It looks good for me.  It is ok to commit it 
> into the trunk.

Thanks everyone involved for fixing this nasty regression!  Much
appreciated.


Segher
diff mbox series

Patch

diff --git a/gcc/ira-costs.cc b/gcc/ira-costs.cc
index d2a801ab9b0..ae8304ff938 100644
--- a/gcc/ira-costs.cc
+++ b/gcc/ira-costs.cc
@@ -1572,12 +1572,16 @@  scan_one_insn (rtx_insn *insn)
       && (! ira_use_lra_p || ! pic_offset_table_rtx
 	  || ! contains_symbol_ref_p (XEXP (note, 0))))
     {
-      /* Costs for NO_REGS are used in cost calculation on the
-	 1st pass when the preferred register classes are not
-	 known yet.  In this case we take the best scenario.  */
-      enum reg_class cl = NO_REGS;
+      enum reg_class cl = GENERAL_REGS;
       rtx reg = SET_DEST (set);
       int num = COST_INDEX (REGNO (reg));
+      /* Costs for NO_REGS are used in cost calculation on the
+	 1st pass when the preferred register classes are not
+	 known yet.  In this case we take the best scenario when
+	 mode can't be put into GENERAL_REGS.  */
+      if (!targetm.hard_regno_mode_ok (ira_class_hard_regs[cl][0],
+				       GET_MODE (reg)))
+	cl = NO_REGS;
 
       COSTS (costs, num)->mem_cost
 	-= ira_memory_move_cost[GET_MODE (reg)][cl][1] * frequency;