diff mbox

[aarch64] Improve TImode constant moves

Message ID 56A4AD68.5090703@redhat.com
State New
Headers show

Commit Message

Richard Henderson Jan. 24, 2016, 10:54 a.m. UTC
This looks to be an incomplete transition of the aarch64 backend to 
CONST_WIDE_INT.  I haven't checked to see if it's a regression from gcc5, but I 
suspect not, since there should have been similar checks for CONST_DOUBLE.

This is probably gcc7 fodder, but it helped me debug another TImode PR.


r~
* config/aarch64/aarch64.c (aarch64_rtx_costs): Handle CONST_WIDE_INT.
	(aarch64_legitimate_constant_p): Accept CONST_SCALAR_INT_P.
	* config/aarch64/predicates.md (aarch64_movti_operand): Accept
	const_wide_int and const_scalar_int_operand.
	(aarch64_reg_or_imm): Likewise.

Comments

Kyrill Tkachov Jan. 25, 2016, 9:32 a.m. UTC | #1
Hi Richard,

On 24/01/16 10:54, Richard Henderson wrote:
> This looks to be an incomplete transition of the aarch64 backend to CONST_WIDE_INT.  I haven't checked to see if it's a regression from gcc5, but I suspect not, since there should have been similar checks for CONST_DOUBLE.
>

FWIW, I defined TARGET_SUPPORTS_WIDE_INT for aarch64 on trunk and the GCC 5 branch in order to fix
PR 68129.

> This is probably gcc7 fodder, but it helped me debug another TImode PR.
>
>
> r~

+    case CONST_WIDE_INT:
+      *cost = 0;
+      for (unsigned int n = CONST_WIDE_INT_NUNITS(x), i = 0; i < n; ++i)
+	{
+	  unsigned HOST_WIDE_INT e = CONST_WIDE_INT_ELT(x, i);
+	  if (e != 0)
+	    *cost += COSTS_N_INSNS (aarch64_internal_mov_immediate
+				    (NULL_RTX, GEN_INT (e), false, DImode));
+	}
+      return true;
+

We usually avoid creating intermediate rtxes in the cost function because
it can potentially be called many times during compilation and we want to avoid
creating too many short-lived objects, though I suppose there's no way getting
around this one (the GEN_INT call).

Thanks,
Kyrill
Richard Henderson Jan. 25, 2016, 4:32 p.m. UTC | #2
On 01/25/2016 01:32 AM, Kyrill Tkachov wrote:
> +    case CONST_WIDE_INT:
> +      *cost = 0;
> +      for (unsigned int n = CONST_WIDE_INT_NUNITS(x), i = 0; i < n; ++i)
> +    {
> +      unsigned HOST_WIDE_INT e = CONST_WIDE_INT_ELT(x, i);
> +      if (e != 0)
> +        *cost += COSTS_N_INSNS (aarch64_internal_mov_immediate
> +                    (NULL_RTX, GEN_INT (e), false, DImode));
> +    }
> +      return true;
> +
> 
> We usually avoid creating intermediate rtxes in the cost function because
> it can potentially be called many times during compilation and we want to avoid
> creating too many short-lived objects, though I suppose there's no way getting
> around this one (the GEN_INT call).

Well, it's only aarch64_internal_mov_immediate -- we could change the interface
to provide the HOST_WIDE_INT value directly.

But that was more than I wanted to do for enabling splittable TImode constants.


r~
James Greenhalgh Feb. 2, 2016, 10:39 a.m. UTC | #3
On Sun, Jan 24, 2016 at 02:54:32AM -0800, Richard Henderson wrote:
> This looks to be an incomplete transition of the aarch64 backend to
> CONST_WIDE_INT.  I haven't checked to see if it's a regression from
> gcc5, but I suspect not, since there should have been similar checks
> for CONST_DOUBLE.
> 
> This is probably gcc7 fodder, but it helped me debug another TImode PR.

When the time comes, this is OK.

Thanks,
James

> 	* config/aarch64/aarch64.c (aarch64_rtx_costs): Handle CONST_WIDE_INT.
> 	(aarch64_legitimate_constant_p): Accept CONST_SCALAR_INT_P.
> 	* config/aarch64/predicates.md (aarch64_movti_operand): Accept
> 	const_wide_int and const_scalar_int_operand.
> 	(aarch64_reg_or_imm): Likewise.
> 
> 
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index df3dec0..38c7443 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -6227,6 +6227,17 @@ aarch64_rtx_costs (rtx x, machine_mode mode, int outer ATTRIBUTE_UNUSED,
>  	}
>        return true;
>  
> +    case CONST_WIDE_INT:
> +      *cost = 0;
> +      for (unsigned int n = CONST_WIDE_INT_NUNITS(x), i = 0; i < n; ++i)
> +	{
> +	  unsigned HOST_WIDE_INT e = CONST_WIDE_INT_ELT(x, i);
> +	  if (e != 0)
> +	    *cost += COSTS_N_INSNS (aarch64_internal_mov_immediate
> +				    (NULL_RTX, GEN_INT (e), false, DImode));
> +	}
> +      return true;
> +
>      case CONST_DOUBLE:
>        if (speed)
>  	{
> @@ -9400,6 +9411,9 @@ aarch64_legitimate_constant_p (machine_mode mode, rtx x)
>        && aarch64_valid_symref (XEXP (x, 0), GET_MODE (XEXP (x, 0))))
>      return true;
>  
> +  if (CONST_SCALAR_INT_P (x))
> +    return true;
> +
>    return aarch64_constant_address_p (x);
>  }
>  
> diff --git a/gcc/config/aarch64/predicates.md b/gcc/config/aarch64/predicates.md
> index e96dc00..3eb33fa 100644
> --- a/gcc/config/aarch64/predicates.md
> +++ b/gcc/config/aarch64/predicates.md
> @@ -217,15 +217,15 @@
>  		 (match_test "aarch64_mov_operand_p (op, mode)")))))
>  
>  (define_predicate "aarch64_movti_operand"
> -  (and (match_code "reg,subreg,mem,const_int")
> +  (and (match_code "reg,subreg,mem,const_int,const_wide_int")
>         (ior (match_operand 0 "register_operand")
>  	    (ior (match_operand 0 "memory_operand")
> -		 (match_operand 0 "const_int_operand")))))
> +		 (match_operand 0 "const_scalar_int_operand")))))
>  
>  (define_predicate "aarch64_reg_or_imm"
> -  (and (match_code "reg,subreg,const_int")
> +  (and (match_code "reg,subreg,const_int,const_wide_int")
>         (ior (match_operand 0 "register_operand")
> -	    (match_operand 0 "const_int_operand"))))
> +	    (match_operand 0 "const_scalar_int_operand"))))
>  
>  ;; True for integer comparisons and for FP comparisons other than LTGT or UNEQ.
>  (define_special_predicate "aarch64_comparison_operator"
diff mbox

Patch

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index df3dec0..38c7443 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -6227,6 +6227,17 @@  aarch64_rtx_costs (rtx x, machine_mode mode, int outer ATTRIBUTE_UNUSED,
 	}
       return true;
 
+    case CONST_WIDE_INT:
+      *cost = 0;
+      for (unsigned int n = CONST_WIDE_INT_NUNITS(x), i = 0; i < n; ++i)
+	{
+	  unsigned HOST_WIDE_INT e = CONST_WIDE_INT_ELT(x, i);
+	  if (e != 0)
+	    *cost += COSTS_N_INSNS (aarch64_internal_mov_immediate
+				    (NULL_RTX, GEN_INT (e), false, DImode));
+	}
+      return true;
+
     case CONST_DOUBLE:
       if (speed)
 	{
@@ -9400,6 +9411,9 @@  aarch64_legitimate_constant_p (machine_mode mode, rtx x)
       && aarch64_valid_symref (XEXP (x, 0), GET_MODE (XEXP (x, 0))))
     return true;
 
+  if (CONST_SCALAR_INT_P (x))
+    return true;
+
   return aarch64_constant_address_p (x);
 }
 
diff --git a/gcc/config/aarch64/predicates.md b/gcc/config/aarch64/predicates.md
index e96dc00..3eb33fa 100644
--- a/gcc/config/aarch64/predicates.md
+++ b/gcc/config/aarch64/predicates.md
@@ -217,15 +217,15 @@ 
 		 (match_test "aarch64_mov_operand_p (op, mode)")))))
 
 (define_predicate "aarch64_movti_operand"
-  (and (match_code "reg,subreg,mem,const_int")
+  (and (match_code "reg,subreg,mem,const_int,const_wide_int")
        (ior (match_operand 0 "register_operand")
 	    (ior (match_operand 0 "memory_operand")
-		 (match_operand 0 "const_int_operand")))))
+		 (match_operand 0 "const_scalar_int_operand")))))
 
 (define_predicate "aarch64_reg_or_imm"
-  (and (match_code "reg,subreg,const_int")
+  (and (match_code "reg,subreg,const_int,const_wide_int")
        (ior (match_operand 0 "register_operand")
-	    (match_operand 0 "const_int_operand"))))
+	    (match_operand 0 "const_scalar_int_operand"))))
 
 ;; True for integer comparisons and for FP comparisons other than LTGT or UNEQ.
 (define_special_predicate "aarch64_comparison_operator"