[PATCH][AArch64] vec_pack_trunc_<mode> should split after register allocator

[PATCH][AArch64] vec_pack_trunc_<mode> should split after register allocator

[Hurugalawadi, Naveen]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]

Hi,

The instruction "vec_pack_trunc_<mode>" should be split after register
allocator for scheduling reasons. Currently the instruction is marked as type
multiple which means it will scheduled as single issued. However, nothing can
be scheduled with either xtn/xtn2 which is a problem in some cases.

The patch splits the instruction and fixes the issue.

Please review the patch and let me know if its okay.
Bootstrapped and Regression tested on aarch64-thunder-linux.

2017-04-27  Naveen H.S  <Naveen.Hurugalawadi@cavium.com>

	* config/aarch64/aarch64-simd.md
	(aarch64_simd_vec_pack_trunc_hi_<mode>): New pattern.
	(vec_pack_trunc_<mode>): Split the instruction pattern.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vec_pack_trunc.patch --]
[-- Type: text/x-patch; name="vec_pack_trunc.patch", Size: 2429 bytes --]

diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md
index c462164..9b5135c 100644
--- a/gcc/config/aarch64/aarch64-simd.md
+++ b/gcc/config/aarch64/aarch64-simd.md
@@ -1278,6 +1278,18 @@
   [(set_attr "type" "neon_shift_imm_narrow_q")]
 )
 
+(define_insn "aarch64_simd_vec_pack_trunc_hi_<mode>"
+ [(set (match_operand:<VNARROWQ2> 0 "register_operand" "=w")
+       (vec_concat:<VNARROWQ2>
+	 (truncate:<VNARROWQ> (match_operand:VQN 1 "register_operand" "w"))
+	 (vec_select:<VNARROWQ>
+	   (match_operand:<VNARROWQ2> 3 "register_operand" "0")
+	   (match_operand:<VNARROWQ2> 2 "vect_par_cnst_hi_half" ""))))]
+ "TARGET_SIMD"
+ "xtn2\\t%0.<V2ntype>, %1.<Vtype>"
+  [(set_attr "type" "neon_shift_imm_narrow_q")]
+)
+
 (define_expand "vec_pack_trunc_<mode>"
  [(match_operand:<VNARROWD> 0 "register_operand" "")
   (match_operand:VDN 1 "register_operand" "")
@@ -1296,17 +1308,41 @@
 
 ;; For quads.
 
-(define_insn "vec_pack_trunc_<mode>"
+(define_insn_and_split "vec_pack_trunc_<mode>"
  [(set (match_operand:<VNARROWQ2> 0 "register_operand" "=&w")
        (vec_concat:<VNARROWQ2>
 	 (truncate:<VNARROWQ> (match_operand:VQN 1 "register_operand" "w"))
 	 (truncate:<VNARROWQ> (match_operand:VQN 2 "register_operand" "w"))))]
  "TARGET_SIMD"
+ "#"
+ "&& reload_completed"
+ [(const_int 0)]
  {
    if (BYTES_BIG_ENDIAN)
-     return "xtn\\t%0.<Vntype>, %2.<Vtype>\;xtn2\\t%0.<V2ntype>, %1.<Vtype>";
+     {
+       rtx low_part = gen_lowpart (<VNARROWQ>mode, operands[0]);
+       emit_insn (gen_aarch64_simd_vec_pack_trunc_<mode> (low_part,
+							  operands[2]));
+       rtx high_part = aarch64_simd_vect_par_cnst_half (<VNARROWQ2>mode,
+							true);
+       emit_insn (gen_aarch64_simd_vec_pack_trunc_hi_<mode> (operands[0],
+							     operands[1],
+							     high_part,
+							     operands[0]));
+     }
    else
-     return "xtn\\t%0.<Vntype>, %1.<Vtype>\;xtn2\\t%0.<V2ntype>, %2.<Vtype>";
+     {
+       rtx low_part = gen_lowpart (<VNARROWQ>mode, operands[0]);
+       emit_insn (gen_aarch64_simd_vec_pack_trunc_<mode> (low_part,
+							  operands[1]));
+       rtx high_part = aarch64_simd_vect_par_cnst_half (<VNARROWQ2>mode,
+							true);
+       emit_insn (gen_aarch64_simd_vec_pack_trunc_hi_<mode> (operands[0],
+							     operands[2],
+							     high_part,
+							     operands[0]));
+     }
+   DONE;
  }
   [(set_attr "type" "multiple")
    (set_attr "length" "8")]

Re: [PING] [PATCH] [AArch64] vec_pack_trunc_<mode> should split after register allocator

[Hurugalawadi, Naveen]

Hi,  

Please consider this as a personal reminder to review the patch
at following link and let me know your comments on the same.  

https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01334.html

Thanks,
Naveen

Re: [PING 2] [PATCH] [AArch64] vec_pack_trunc_<mode> should split after register allocator

[Hurugalawadi, Naveen]

Hi,  

Please consider this as a personal reminder to review the patch
at following link and let me know your comments on the same.  

https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01334.html

Thanks,
Naveen



    

Re: [PING 3] [PATCH] [AArch64] vec_pack_trunc_<mode> should split after register allocator

[Hurugalawadi, Naveen]

Hi,  

Please consider this as a personal reminder to review the patch
at following link and let me know your comments on the same.  

https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01334.html

Thanks,
Naveen



        

Re: [PING 4] [PATCH] [AArch64] vec_pack_trunc_<mode> should split after register allocator

[Hurugalawadi, Naveen]

Hi,  

Please consider this as a personal reminder to review the patch
at following link and let me know your comments on the same.  

https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01334.html

Thanks,
Naveen



            

Re: [PING 5] [PATCH] [AArch64] vec_pack_trunc_<mode> should split after register allocator

[Hurugalawadi, Naveen]

Hi,  

Please consider this as a personal reminder to review the patch
at following link and let me know your comments on the same.  

https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01334.html

Thanks,
Naveen



                

Re: [PATCH][AArch64] vec_pack_trunc_<mode> should split after register allocator

[James Greenhalgh]

On Thu, Apr 27, 2017 at 05:08:38AM +0000, Hurugalawadi, Naveen wrote:
> Hi,
> 
> The instruction "vec_pack_trunc_<mode>" should be split after register
> allocator for scheduling reasons. Currently the instruction is marked as type
> multiple which means it will scheduled as single issued. However, nothing can
> be scheduled with either xtn/xtn2 which is a problem in some cases.

What's the reason for splitting this only after reload? I think we can
split this whenever we like, and that there isn't any benefit in keeping
the pair together?

Am I missing something?

Thanks,
James

> 
> The patch splits the instruction and fixes the issue.
> 
> Please review the patch and let me know if its okay.
> Bootstrapped and Regression tested on aarch64-thunder-linux.
> 
> 2017-04-27  Naveen H.S  <Naveen.Hurugalawadi@cavium.com>
> 
> 	* config/aarch64/aarch64-simd.md
> 	(aarch64_simd_vec_pack_trunc_hi_<mode>): New pattern.
> 	(vec_pack_trunc_<mode>): Split the instruction pattern.

Re: [PATCH][AArch64] vec_pack_trunc_<mode> should split after register allocator

[Hurugalawadi, Naveen]

[-- Attachment #1: Type: text/plain, Size: 350 bytes --]

Hi,

>> I think we can split this whenever we like, and
>> that there isn't any benefit in keeping the pair together?

Thanks for the review and your views.

The patch is modified as per your suggestion.

Please review the patch and let me know if its okay?

Bootstrapped and Regression done on AArch64-Thunder-Linux.

Thanks,
Naveen   

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vec_pack-1.patch --]
[-- Type: text/x-diff; name="vec_pack-1.patch", Size: 2604 bytes --]

diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md
index 1cb6eeb..a41edad 100644
--- a/gcc/config/aarch64/aarch64-simd.md
+++ b/gcc/config/aarch64/aarch64-simd.md
@@ -1291,6 +1291,18 @@
   [(set_attr "type" "neon_shift_imm_narrow_q")]
 )
 
+(define_insn "aarch64_simd_vec_pack_trunc_hi_<mode>"
+ [(set (match_operand:<VNARROWQ2> 0 "register_operand" "=w")
+       (vec_concat:<VNARROWQ2>
+	 (truncate:<VNARROWQ> (match_operand:VQN 1 "register_operand" "w"))
+	 (vec_select:<VNARROWQ>
+	   (match_operand:<VNARROWQ2> 3 "register_operand" "0")
+	   (match_operand:<VNARROWQ2> 2 "vect_par_cnst_hi_half" ""))))]
+ "TARGET_SIMD"
+ "xtn2\\t%0.<V2ntype>, %1.<Vtype>"
+  [(set_attr "type" "neon_shift_imm_narrow_q")]
+)
+
 (define_expand "vec_pack_trunc_<mode>"
  [(match_operand:<VNARROWD> 0 "register_operand" "")
   (match_operand:VDN 1 "register_operand" "")
@@ -1309,20 +1321,39 @@
 
 ;; For quads.
 
-(define_insn "vec_pack_trunc_<mode>"
- [(set (match_operand:<VNARROWQ2> 0 "register_operand" "=&w")
+(define_expand "vec_pack_trunc_<mode>"
+ [(set (match_operand:<VNARROWQ2> 0 "register_operand" "")
        (vec_concat:<VNARROWQ2>
-	 (truncate:<VNARROWQ> (match_operand:VQN 1 "register_operand" "w"))
-	 (truncate:<VNARROWQ> (match_operand:VQN 2 "register_operand" "w"))))]
+	 (truncate:<VNARROWQ> (match_operand:VQN 1 "register_operand" ""))
+	 (truncate:<VNARROWQ> (match_operand:VQN 2 "register_operand" ""))))]
  "TARGET_SIMD"
  {
    if (BYTES_BIG_ENDIAN)
-     return "xtn\\t%0.<Vntype>, %2.<Vtype>\;xtn2\\t%0.<V2ntype>, %1.<Vtype>";
+     {
+       rtx low_part = gen_lowpart (<VNARROWQ>mode, operands[0]);
+       emit_insn (gen_aarch64_simd_vec_pack_trunc_<mode> (low_part,
+							  operands[2]));
+       rtx high_part = aarch64_simd_vect_par_cnst_half (<VNARROWQ2>mode,
+							true);
+       emit_insn (gen_aarch64_simd_vec_pack_trunc_hi_<mode> (operands[0],
+							     operands[1],
+							     high_part,
+							     operands[0]));
+     }
    else
-     return "xtn\\t%0.<Vntype>, %1.<Vtype>\;xtn2\\t%0.<V2ntype>, %2.<Vtype>";
+     {
+       rtx low_part = gen_lowpart (<VNARROWQ>mode, operands[0]);
+       emit_insn (gen_aarch64_simd_vec_pack_trunc_<mode> (low_part,
+							  operands[1]));
+       rtx high_part = aarch64_simd_vect_par_cnst_half (<VNARROWQ2>mode,
+							true);
+       emit_insn (gen_aarch64_simd_vec_pack_trunc_hi_<mode> (operands[0],
+							     operands[2],
+							     high_part,
+							     operands[0]));
+     }
+   DONE;
  }
-  [(set_attr "type" "multiple")
-   (set_attr "length" "8")]
 )
 
 ;; Widening operations.

Re: [PATCH][AArch64] vec_pack_trunc_<mode> should split after register allocator

[James Greenhalgh]

On Tue, Jul 25, 2017 at 07:30:49AM +0000, Hurugalawadi, Naveen wrote:
> Hi,
> 
> >> I think we can split this whenever we like, and
> >> that there isn't any benefit in keeping the pair together?
> 
> Thanks for the review and your views.

Thanks for the updated patch, and sorry that I haven't been clear in what
I was asking for.

I was wondering why we could not use an insn_and_split without the
reload_completed guard - there is probably still value in allowing other
optimisation passes to see that we can support:

   [(set (match_operand:<VNARROWQ2> 0 "register_operand" "")
         (vec_concat:<VNARROWQ2>
	   (truncate:<VNARROWQ> (match_operand:VQN 1 "register_operand" "w"))
	   (truncate:<VNARROWQ> (match_operand:VQN 2 "register_operand" "w"))))]

but an expand pattern is not going to permit that.

Could you switch this back to an insn_and_split as it was in the previous
patch, and just drop the && reload_completed ?

Thanks,
James

> 
> The patch is modified as per your suggestion.
> 
> Please review the patch and let me know if its okay?
> 
> Bootstrapped and Regression done on AArch64-Thunder-Linux.
> 
> Thanks,
> Naveen   

> diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md
> index 1cb6eeb..a41edad 100644
> --- a/gcc/config/aarch64/aarch64-simd.md
> +++ b/gcc/config/aarch64/aarch64-simd.md
> @@ -1291,6 +1291,18 @@
>    [(set_attr "type" "neon_shift_imm_narrow_q")]
>  )
>  
> +(define_insn "aarch64_simd_vec_pack_trunc_hi_<mode>"
> + [(set (match_operand:<VNARROWQ2> 0 "register_operand" "=w")
> +       (vec_concat:<VNARROWQ2>
> +	 (truncate:<VNARROWQ> (match_operand:VQN 1 "register_operand" "w"))
> +	 (vec_select:<VNARROWQ>
> +	   (match_operand:<VNARROWQ2> 3 "register_operand" "0")
> +	   (match_operand:<VNARROWQ2> 2 "vect_par_cnst_hi_half" ""))))]
> + "TARGET_SIMD"
> + "xtn2\\t%0.<V2ntype>, %1.<Vtype>"
> +  [(set_attr "type" "neon_shift_imm_narrow_q")]
> +)
> +
>  (define_expand "vec_pack_trunc_<mode>"
>   [(match_operand:<VNARROWD> 0 "register_operand" "")
>    (match_operand:VDN 1 "register_operand" "")
> @@ -1309,20 +1321,39 @@
>  
>  ;; For quads.
>  
> -(define_insn "vec_pack_trunc_<mode>"
> - [(set (match_operand:<VNARROWQ2> 0 "register_operand" "=&w")
> +(define_expand "vec_pack_trunc_<mode>"
> + [(set (match_operand:<VNARROWQ2> 0 "register_operand" "")
>         (vec_concat:<VNARROWQ2>
> -	 (truncate:<VNARROWQ> (match_operand:VQN 1 "register_operand" "w"))
> -	 (truncate:<VNARROWQ> (match_operand:VQN 2 "register_operand" "w"))))]
> +	 (truncate:<VNARROWQ> (match_operand:VQN 1 "register_operand" ""))
> +	 (truncate:<VNARROWQ> (match_operand:VQN 2 "register_operand" ""))))]
>   "TARGET_SIMD"
>   {
>     if (BYTES_BIG_ENDIAN)
> -     return "xtn\\t%0.<Vntype>, %2.<Vtype>\;xtn2\\t%0.<V2ntype>, %1.<Vtype>";
> +     {
> +       rtx low_part = gen_lowpart (<VNARROWQ>mode, operands[0]);
> +       emit_insn (gen_aarch64_simd_vec_pack_trunc_<mode> (low_part,
> +							  operands[2]));
> +       rtx high_part = aarch64_simd_vect_par_cnst_half (<VNARROWQ2>mode,
> +							true);
> +       emit_insn (gen_aarch64_simd_vec_pack_trunc_hi_<mode> (operands[0],
> +							     operands[1],
> +							     high_part,
> +							     operands[0]));
> +     }
>     else
> -     return "xtn\\t%0.<Vntype>, %1.<Vtype>\;xtn2\\t%0.<V2ntype>, %2.<Vtype>";
> +     {
> +       rtx low_part = gen_lowpart (<VNARROWQ>mode, operands[0]);
> +       emit_insn (gen_aarch64_simd_vec_pack_trunc_<mode> (low_part,
> +							  operands[1]));
> +       rtx high_part = aarch64_simd_vect_par_cnst_half (<VNARROWQ2>mode,
> +							true);
> +       emit_insn (gen_aarch64_simd_vec_pack_trunc_hi_<mode> (operands[0],
> +							     operands[2],
> +							     high_part,
> +							     operands[0]));
> +     }
> +   DONE;
>   }
> -  [(set_attr "type" "multiple")
> -   (set_attr "length" "8")]
>  )
>  
>  ;; Widening operations.

Re: [PATCH][AArch64] vec_pack_trunc_<mode> should split after register allocator

[Hurugalawadi, Naveen]

[-- Attachment #1: Type: text/plain, Size: 433 bytes --]

Hi,

>> I haven't been clear in what I was asking for

Sorry. We understood right with the first comment but the second
part confused us a bit :).

>> Could you switch this back to an insn_and_split as it was in the previous
>> patch, and just drop the && reload_completed ?

Done.

Bootstrapped and Regression done on AArch64-Thunder-Linux.
Please review the patch and let me know if its okay?

Thanks,
Naveen   


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vec_pack-2.patch --]
[-- Type: text/x-diff; name="vec_pack-2.patch", Size: 2410 bytes --]

diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md
index 1cb6eeb..0011040 100644
--- a/gcc/config/aarch64/aarch64-simd.md
+++ b/gcc/config/aarch64/aarch64-simd.md
@@ -1291,6 +1291,18 @@
   [(set_attr "type" "neon_shift_imm_narrow_q")]
 )
 
+(define_insn "aarch64_simd_vec_pack_trunc_hi_<mode>"
+ [(set (match_operand:<VNARROWQ2> 0 "register_operand" "=w")
+       (vec_concat:<VNARROWQ2>
+	 (truncate:<VNARROWQ> (match_operand:VQN 1 "register_operand" "w"))
+	 (vec_select:<VNARROWQ>
+	   (match_operand:<VNARROWQ2> 3 "register_operand" "0")
+	   (match_operand:<VNARROWQ2> 2 "vect_par_cnst_hi_half" ""))))]
+ "TARGET_SIMD"
+ "xtn2\\t%0.<V2ntype>, %1.<Vtype>"
+  [(set_attr "type" "neon_shift_imm_narrow_q")]
+)
+
 (define_expand "vec_pack_trunc_<mode>"
  [(match_operand:<VNARROWD> 0 "register_operand" "")
   (match_operand:VDN 1 "register_operand" "")
@@ -1309,17 +1321,41 @@
 
 ;; For quads.
 
-(define_insn "vec_pack_trunc_<mode>"
+(define_insn_and_split "vec_pack_trunc_<mode>"
  [(set (match_operand:<VNARROWQ2> 0 "register_operand" "=&w")
        (vec_concat:<VNARROWQ2>
 	 (truncate:<VNARROWQ> (match_operand:VQN 1 "register_operand" "w"))
 	 (truncate:<VNARROWQ> (match_operand:VQN 2 "register_operand" "w"))))]
  "TARGET_SIMD"
+ "#"
+ ""
+ [(const_int 0)]
  {
    if (BYTES_BIG_ENDIAN)
-     return "xtn\\t%0.<Vntype>, %2.<Vtype>\;xtn2\\t%0.<V2ntype>, %1.<Vtype>";
+     {
+       rtx low_part = gen_lowpart (<VNARROWQ>mode, operands[0]);
+       emit_insn (gen_aarch64_simd_vec_pack_trunc_<mode> (low_part,
+							  operands[2]));
+       rtx high_part = aarch64_simd_vect_par_cnst_half (<VNARROWQ2>mode,
+							true);
+       emit_insn (gen_aarch64_simd_vec_pack_trunc_hi_<mode> (operands[0],
+							     operands[1],
+							     high_part,
+							     operands[0]));
+     }
    else
-     return "xtn\\t%0.<Vntype>, %1.<Vtype>\;xtn2\\t%0.<V2ntype>, %2.<Vtype>";
+     {
+       rtx low_part = gen_lowpart (<VNARROWQ>mode, operands[0]);
+       emit_insn (gen_aarch64_simd_vec_pack_trunc_<mode> (low_part,
+							  operands[1]));
+       rtx high_part = aarch64_simd_vect_par_cnst_half (<VNARROWQ2>mode,
+							true);
+       emit_insn (gen_aarch64_simd_vec_pack_trunc_hi_<mode> (operands[0],
+							     operands[2],
+							     high_part,
+							     operands[0]));
+     }
+   DONE;
  }
   [(set_attr "type" "multiple")
    (set_attr "length" "8")]

Re: [PING] [PATCH] [AArch64] vec_pack_trunc_<mode> should split after register allocator

[Hurugalawadi, Naveen]

Hi,  

Please consider this as a personal reminder to review the patch
at following link and let me know your comments on the same.  

https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01529.html

Thanks,
Naveen