LoongArch: Fix cost model for alsl

Message ID 20250115101018.3173591-2-xry111@xry111.site
State Committed
Commit ce36692f8e10a619563938851f507cdb15567cf8
Headers
Series LoongArch: Fix cost model for alsl |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed

Commit Message

Xi Ruoyao Jan. 15, 2025, 10:10 a.m. UTC
  Our cost model for alsl was wrong: it matches (a + b * imm) where imm is
1, 2, 3, or 4 (should be 2, 4, 8, or 16), and it does not match
(a + (b << imm)) at all.  For the test case:

    a += c << 3;
    b += c << 3;

it caused the compiler to perform a CSE and make one slli and two add,
but we just want two alsl.

gcc/ChangeLog:

	* config/loongarch/loongarch.cc (loongarch_rtx_costs): Fix the
	cost for (a + b * imm) and (a + b << imm) which can be
	implemented with a single alsl instruction.

gcc/testsuite/ChangeLog:

	* gcc.target/loongarch/alsl-cost.c: New test.
---

Bootstrapped & regtested on loongarch64-linux-gnu.  Ok for trunk?

 gcc/config/loongarch/loongarch.cc             | 27 +++++++++++++++----
 .../gcc.target/loongarch/alsl-cost.c          | 14 ++++++++++
 2 files changed, 36 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/loongarch/alsl-cost.c
  

Comments

Lulu Cheng Jan. 16, 2025, 12:30 p.m. UTC | #1
在 2025/1/15 下午6:10, Xi Ruoyao 写道:
> diff --git a/gcc/config/loongarch/loongarch.cc b/gcc/config/loongarch/loongarch.cc
> index 9d97f0216f0..3a8e1297bd3 100644
> --- a/gcc/config/loongarch/loongarch.cc
> +++ b/gcc/config/loongarch/loongarch.cc
> @@ -3929,14 +3929,31 @@ loongarch_rtx_costs (rtx x, machine_mode mode, int outer_code,
>   
>         /* If it's an add + mult (which is equivalent to shift left) and
>   	 it's immediate operand satisfies const_immalsl_operand predicate.  */
> -      if ((mode == SImode || (TARGET_64BIT && mode == DImode))
> -	  && GET_CODE (XEXP (x, 0)) == MULT)
> +      if (code == PLUS

Hi,

This section of code is already within the "case PLUS" block, so I think 
the condition "code == PLUS" is unnecessary.

Otherwise LGTM.

Thanks!

> +	  && (mode == SImode || (TARGET_64BIT && mode == DImode)))
>   	{
> -	  rtx op2 = XEXP (XEXP (x, 0), 1);
> -	  if (const_immalsl_operand (op2, mode))
> +	  HOST_WIDE_INT shamt = -1;
> +	  rtx lhs = XEXP (x, 0);
> +	  rtx_code code_lhs = GET_CODE (lhs);
> +
> +	  switch (code_lhs)
> +	    {
> +	    case ASHIFT:
> +	      if (CONST_INT_P (XEXP (lhs, 1)))
> +		shamt = INTVAL (XEXP (lhs, 1));
> +	      break;
> +	    case MULT:
> +	      if (CONST_INT_P (XEXP (lhs, 1)))
> +		shamt = exact_log2 (INTVAL (XEXP (lhs, 1)));
> +	      break;
> +	    default:
> +	      break;
> +	    }
> +
> +	  if (IN_RANGE (shamt, 1, 4))
>   	    {
>   	      *total = (COSTS_N_INSNS (1)
> -			+ set_src_cost (XEXP (XEXP (x, 0), 0), mode, speed)
> +			+ set_src_cost (XEXP (lhs, 0), mode, speed)
>   			+ set_src_cost (XEXP (x, 1), mode, speed));
>   	      return true;
>   	    }
> diff --git a/gcc/testsuite/gcc.target/loongarch/alsl-cost.c b/gcc/testsuite/gcc.target/loongarch/alsl-cost.c
> new file mode 100644
> index 00000000000..a182279015c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/loongarch/alsl-cost.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mtune=loongarch64" } */
> +/* { dg-final { scan-assembler-times "alsl\\\.\[wd\]" 2 } } */
> +
> +struct P
> +{
> +  long a, b;
> +};
> +
> +struct P
> +t (struct P x, long n)
> +{
> +  return (struct P){.a = x.a + n * 8, .b = x.b + n * 8};
> +}
  
Xi Ruoyao Jan. 16, 2025, 12:52 p.m. UTC | #2
On Thu, 2025-01-16 at 20:30 +0800, Lulu Cheng wrote:
> 
> 在 2025/1/15 下午6:10, Xi Ruoyao 写道:
> > diff --git a/gcc/config/loongarch/loongarch.cc b/gcc/config/loongarch/loongarch.cc
> > index 9d97f0216f0..3a8e1297bd3 100644
> > --- a/gcc/config/loongarch/loongarch.cc
> > +++ b/gcc/config/loongarch/loongarch.cc
> > @@ -3929,14 +3929,31 @@ loongarch_rtx_costs (rtx x, machine_mode mode, int outer_code,
> >   
> >         /* If it's an add + mult (which is equivalent to shift left) and
> >   	 it's immediate operand satisfies const_immalsl_operand predicate.  */
> > -      if ((mode == SImode || (TARGET_64BIT && mode == DImode))
> > -	  && GET_CODE (XEXP (x, 0)) == MULT)
> > +      if (code == PLUS
> 
> Hi,
> 
> This section of code is already within the "case PLUS" block, so I think 
> the condition "code == PLUS" is unnecessary.

The reason is "case MINUS:" falls through into "case PLUS:" and we don't
have a "slsl" instruction, so we need the check to reject things like a
- b * 4 here.
  
Xi Ruoyao Jan. 16, 2025, 12:59 p.m. UTC | #3
On Thu, 2025-01-16 at 20:52 +0800, Xi Ruoyao wrote:
> On Thu, 2025-01-16 at 20:30 +0800, Lulu Cheng wrote:
> > 
> > 在 2025/1/15 下午6:10, Xi Ruoyao 写道:
> > > diff --git a/gcc/config/loongarch/loongarch.cc b/gcc/config/loongarch/loongarch.cc
> > > index 9d97f0216f0..3a8e1297bd3 100644
> > > --- a/gcc/config/loongarch/loongarch.cc
> > > +++ b/gcc/config/loongarch/loongarch.cc
> > > @@ -3929,14 +3929,31 @@ loongarch_rtx_costs (rtx x, machine_mode mode, int outer_code,
> > >   
> > >         /* If it's an add + mult (which is equivalent to shift left) and
> > >   	 it's immediate operand satisfies const_immalsl_operand predicate.  */
> > > -      if ((mode == SImode || (TARGET_64BIT && mode == DImode))
> > > -	  && GET_CODE (XEXP (x, 0)) == MULT)
> > > +      if (code == PLUS
> > 
> > Hi,
> > 
> > This section of code is already within the "case PLUS" block, so I think 
> > the condition "code == PLUS" is unnecessary.
> 
> The reason is "case MINUS:" falls through into "case PLUS:" and we don't
> have a "slsl" instruction, so we need the check to reject things like a
> - b * 4 here.

Pressed "sent" too early: I think I need to mention this in the
changelog indeed.
  
Lulu Cheng Jan. 16, 2025, 1:07 p.m. UTC | #4
在 2025/1/16 下午8:59, Xi Ruoyao 写道:
> On Thu, 2025-01-16 at 20:52 +0800, Xi Ruoyao wrote:
>> On Thu, 2025-01-16 at 20:30 +0800, Lulu Cheng wrote:
>>> 在 2025/1/15 下午6:10, Xi Ruoyao 写道:
>>>> diff --git a/gcc/config/loongarch/loongarch.cc b/gcc/config/loongarch/loongarch.cc
>>>> index 9d97f0216f0..3a8e1297bd3 100644
>>>> --- a/gcc/config/loongarch/loongarch.cc
>>>> +++ b/gcc/config/loongarch/loongarch.cc
>>>> @@ -3929,14 +3929,31 @@ loongarch_rtx_costs (rtx x, machine_mode mode, int outer_code,
>>>>    
>>>>          /* If it's an add + mult (which is equivalent to shift left) and
>>>>    	 it's immediate operand satisfies const_immalsl_operand predicate.  */
>>>> -      if ((mode == SImode || (TARGET_64BIT && mode == DImode))
>>>> -	  && GET_CODE (XEXP (x, 0)) == MULT)
>>>> +      if (code == PLUS
>>> Hi,
>>>
>>> This section of code is already within the "case PLUS" block, so I think
>>> the condition "code == PLUS" is unnecessary.
>> The reason is "case MINUS:" falls through into "case PLUS:" and we don't
>> have a "slsl" instruction, so we need the check to reject things like a
>> - b * 4 here.
> Pressed "sent" too early: I think I need to mention this in the
> changelog indeed.
>
It's my fault for not reading carefully enough; I missed the "case 
MINUS:". I have no other questions now.
  

Patch

diff --git a/gcc/config/loongarch/loongarch.cc b/gcc/config/loongarch/loongarch.cc
index 9d97f0216f0..3a8e1297bd3 100644
--- a/gcc/config/loongarch/loongarch.cc
+++ b/gcc/config/loongarch/loongarch.cc
@@ -3929,14 +3929,31 @@  loongarch_rtx_costs (rtx x, machine_mode mode, int outer_code,
 
       /* If it's an add + mult (which is equivalent to shift left) and
 	 it's immediate operand satisfies const_immalsl_operand predicate.  */
-      if ((mode == SImode || (TARGET_64BIT && mode == DImode))
-	  && GET_CODE (XEXP (x, 0)) == MULT)
+      if (code == PLUS
+	  && (mode == SImode || (TARGET_64BIT && mode == DImode)))
 	{
-	  rtx op2 = XEXP (XEXP (x, 0), 1);
-	  if (const_immalsl_operand (op2, mode))
+	  HOST_WIDE_INT shamt = -1;
+	  rtx lhs = XEXP (x, 0);
+	  rtx_code code_lhs = GET_CODE (lhs);
+
+	  switch (code_lhs)
+	    {
+	    case ASHIFT:
+	      if (CONST_INT_P (XEXP (lhs, 1)))
+		shamt = INTVAL (XEXP (lhs, 1));
+	      break;
+	    case MULT:
+	      if (CONST_INT_P (XEXP (lhs, 1)))
+		shamt = exact_log2 (INTVAL (XEXP (lhs, 1)));
+	      break;
+	    default:
+	      break;
+	    }
+
+	  if (IN_RANGE (shamt, 1, 4))
 	    {
 	      *total = (COSTS_N_INSNS (1)
-			+ set_src_cost (XEXP (XEXP (x, 0), 0), mode, speed)
+			+ set_src_cost (XEXP (lhs, 0), mode, speed)
 			+ set_src_cost (XEXP (x, 1), mode, speed));
 	      return true;
 	    }
diff --git a/gcc/testsuite/gcc.target/loongarch/alsl-cost.c b/gcc/testsuite/gcc.target/loongarch/alsl-cost.c
new file mode 100644
index 00000000000..a182279015c
--- /dev/null
+++ b/gcc/testsuite/gcc.target/loongarch/alsl-cost.c
@@ -0,0 +1,14 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -mtune=loongarch64" } */
+/* { dg-final { scan-assembler-times "alsl\\\.\[wd\]" 2 } } */
+
+struct P
+{
+  long a, b;
+};
+
+struct P
+t (struct P x, long n)
+{
+  return (struct P){.a = x.a + n * 8, .b = x.b + n * 8};
+}