[PATCH] [AArch64] PR target/71663 Improve Vector Initializtion

[PATCH] [AArch64] PR target/71663 Improve Vector Initializtion

[Hurugalawadi, Naveen]

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

Hi,

The AArch64 vector initialization sequence can be optimized to generate
better code. The attached patch handles for the case where the vector
contains only variables. It checks for the common elements in the vector
and inserts the values in optimized way.

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

2016-12-09  Andrew PInski  <apinski@cavium.com>

gcc
	* config/aarch64/aarch64.c (aarch64_expand_vector_init):
	Improve vector initialization code gen.

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

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index e87831f..da5b6fa 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -11609,11 +11609,54 @@ aarch64_expand_vector_init (rtx target, rtx vals)
       aarch64_expand_vector_init (target, copy);
     }
 
-  /* Insert the variable lanes directly.  */
-
   enum insn_code icode = optab_handler (vec_set_optab, mode);
   gcc_assert (icode != CODE_FOR_nothing);
 
+  /* If there is only varables, try to optimize
+     the inseration using dup for the most common element
+     followed by insertations. */
+  if (n_var == n_elts && n_elts <= 16)
+    {
+      int matches[16][2];
+      int nummatches = 0;
+      memset (matches, 0, sizeof(matches));
+      for(int i = 0; i < n_elts; i++)
+	{
+	  for (int j = 0; j <= i; j++)
+	    {
+	      if (rtx_equal_p (XVECEXP (vals, 0, i), XVECEXP (vals, 0, j)))
+		{
+		  matches[i][0] = j;
+		  matches[j][1]++;
+		  if (i != j)
+		    nummatches++;
+		  break;
+		}
+	    }
+	}
+      int maxelement = 0;
+      int maxv = 0;
+      for (int i = 0; i < n_elts; i++)
+	if (matches[i][1] > maxv)
+	  maxelement = i, maxv = matches[i][1];
+
+      /* Create a duplicate of the most common element. */
+      rtx x = copy_to_mode_reg (inner_mode, XVECEXP (vals, 0, maxelement));
+      aarch64_emit_move (target, gen_rtx_VEC_DUPLICATE (mode, x));
+      /* Insert the rest. */
+      for (int i = 0; i < n_elts; i++)
+	{
+	  rtx x = XVECEXP (vals, 0, i);
+	  if (matches[i][0] == maxelement)
+	    continue;
+	  x = copy_to_mode_reg (inner_mode, x);
+	  emit_insn (GEN_FCN (icode) (target, x, GEN_INT (i)));
+	}
+      return;
+    }
+
+  /* Insert the variable lanes directly.  */
+
   for (int i = 0; i < n_elts; i++)
     {
       rtx x = XVECEXP (vals, 0, i);

Re: [PATCH] [AArch64] PR target/71663 Improve Vector Initializtion

[Hurugalawadi, Naveen]

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

Hi,

Sorry. Missed out the testcase in patch submission.
Added the missing testcase along with the ChangeLog.
Please review the same and let us know if thats okay?

2016-12-09  Andrew PInski  <apinski@cavium.com>

gcc
        * config/aarch64/aarch64.c (aarch64_expand_vector_init):
        Improve vector initialization code gen.    
gcc/testsuite
        * gcc.target/aarch64/pr71663.c: New Testcase.

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

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index e87831f..da5b6fa 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -11609,11 +11609,54 @@ aarch64_expand_vector_init (rtx target, rtx vals)
       aarch64_expand_vector_init (target, copy);
     }
 
-  /* Insert the variable lanes directly.  */
-
   enum insn_code icode = optab_handler (vec_set_optab, mode);
   gcc_assert (icode != CODE_FOR_nothing);
 
+  /* If there is only varables, try to optimize
+     the inseration using dup for the most common element
+     followed by insertations. */
+  if (n_var == n_elts && n_elts <= 16)
+    {
+      int matches[16][2];
+      int nummatches = 0;
+      memset (matches, 0, sizeof(matches));
+      for(int i = 0; i < n_elts; i++)
+	{
+	  for (int j = 0; j <= i; j++)
+	    {
+	      if (rtx_equal_p (XVECEXP (vals, 0, i), XVECEXP (vals, 0, j)))
+		{
+		  matches[i][0] = j;
+		  matches[j][1]++;
+		  if (i != j)
+		    nummatches++;
+		  break;
+		}
+	    }
+	}
+      int maxelement = 0;
+      int maxv = 0;
+      for (int i = 0; i < n_elts; i++)
+	if (matches[i][1] > maxv)
+	  maxelement = i, maxv = matches[i][1];
+
+      /* Create a duplicate of the most common element. */
+      rtx x = copy_to_mode_reg (inner_mode, XVECEXP (vals, 0, maxelement));
+      aarch64_emit_move (target, gen_rtx_VEC_DUPLICATE (mode, x));
+      /* Insert the rest. */
+      for (int i = 0; i < n_elts; i++)
+	{
+	  rtx x = XVECEXP (vals, 0, i);
+	  if (matches[i][0] == maxelement)
+	    continue;
+	  x = copy_to_mode_reg (inner_mode, x);
+	  emit_insn (GEN_FCN (icode) (target, x, GEN_INT (i)));
+	}
+      return;
+    }
+
+  /* Insert the variable lanes directly.  */
+
   for (int i = 0; i < n_elts; i++)
     {
       rtx x = XVECEXP (vals, 0, i);
diff --git a/gcc/testsuite/gcc.target/aarch64/pr71663.c b/gcc/testsuite/gcc.target/aarch64/pr71663.c
new file mode 100644
index 0000000..c8df847
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr71663.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#define vector __attribute__((vector_size(16)))
+
+vector float combine (float a, float b, float c, float d)
+{
+  return (vector float) { a, b, c, d };
+}
+
+/* { dg-final { scan-assembler-not "movi\t" } } */
+/* { dg-final { scan-assembler-not "orr\t" } } */
+/* { dg-final { scan-assembler-times "ins\t" 3 } } */
+/* { dg-final { scan-assembler-times "dup\t" 1 } } */

[PATCH] [AArch64] Implement popcount pattern

[Hurugalawadi, Naveen]

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

Hi,

Please find attached the patch that implements the support for popcount
patterns in AArch64.

The implementation improves OVS-DPDK on ThunderX by 3%. It would have a
similar effect on other AArch64 targets.

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

2016-12-12  Andrew Pinski  <apinski@cavium.com>

gcc
	* config/aarch64/aarch64.md (popcount<mode>2): New pattern.

gcc/testsuite
	* gcc.target/aarch64/popcount.c : New Testcase.

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

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 65eb326..c688ddc 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -3785,6 +3785,39 @@
   }
 )
 
+/* Pop count be done via the pop count instruction in NEON. */
+/*
+  mov v.1d, x0
+  Cnt v1.8b, v.8b
+  Addv b2, v1.8b
+  Mov w0, v2.b[0]
+*/
+(define_expand "popcount<mode>2"
+  [(match_operand:GPI 0 "register_operand")
+   (match_operand:GPI 1 "register_operand")]
+  "TARGET_SIMD"
+{
+  rtx v = gen_reg_rtx (V8QImode);
+  rtx v1 = gen_reg_rtx (V8QImode);
+  rtx r = gen_reg_rtx (QImode);
+  rtx in = operands[1];
+  rtx out = operands[0];
+  if(<MODE>mode == SImode)
+    {
+      rtx tmp;
+      tmp = gen_reg_rtx (DImode);
+      /* If we have SImode, zero extend to DImode, pop count does
+         not change if we have extra zeros. */
+      emit_insn (gen_zero_extendsidi2 (tmp, in));
+      in = tmp;
+    }
+  emit_move_insn (v, gen_lowpart (V8QImode, in));
+  emit_insn (gen_popcountv8qi2 (v1, v));
+  emit_insn (gen_reduc_plus_scal_v8qi (r, v1));
+  emit_insn (gen_zero_extendqi<mode>2 (out, r));
+  DONE;
+})
+
 (define_insn "clrsb<mode>2"
   [(set (match_operand:GPI 0 "register_operand" "=r")
         (clrsb:GPI (match_operand:GPI 1 "register_operand" "r")))]
diff --git a/gcc/testsuite/gcc.target/aarch64/popcount.c b/gcc/testsuite/gcc.target/aarch64/popcount.c
new file mode 100644
index 0000000..2d71168
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/popcount.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+int foo(int x)
+{
+  return __builtin_popcount(x);
+}
+
+/* { dg-final { scan-assembler-not "popcountdi2" } } */
+/* { dg-final { scan-assembler "cnt\t" } } */

Re: [PATCH] [AArch64] Implement popcount pattern

[Kyrill Tkachov]

Hi Naveen,

On 12/12/16 03:16, Hurugalawadi, Naveen wrote:
> Hi,
>
> Please find attached the patch that implements the support for popcount
> patterns in AArch64.
>
> The implementation improves OVS-DPDK on ThunderX by 3%. It would have a
> similar effect on other AArch64 targets.
>
> Please review the patch and let us know if its okay?

Besides a few comments below this looks good to me if it has gone through
the normal required bootstrap and testing, but I can't approve.
It's at the discretion of the target maintainers/reviewers if they want to
accept it at this stage.

> 2016-12-12  Andrew Pinski  <apinski@cavium.com>
>
> gcc
> 	* config/aarch64/aarch64.md (popcount<mode>2): New pattern.
>
> gcc/testsuite
> 	* gcc.target/aarch64/popcount.c : New Testcase.

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 65eb326..c688ddc 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -3785,6 +3785,39 @@
    }
  )
  
+/* Pop count be done via the pop count instruction in NEON. */

The rest of the MD file uses the term AdvSIMD. Also, the instrurction
is CNT rather than "pop count".

+/*
+  mov v.1d, x0
+  Cnt v1.8b, v.8b
+  Addv b2, v1.8b
+  Mov w0, v2.b[0]
+*/

Minor nit, but please use semicolons for MD file comments. The convention (at least in the aarch64 md files)
is to use ";;" at the beginning of the line. Also, please use uppercase letters for the assembly instructions
in the comment.

+(define_expand "popcount<mode>2"
+  [(match_operand:GPI 0 "register_operand")
+   (match_operand:GPI 1 "register_operand")]
+  "TARGET_SIMD"
+{
+  rtx v = gen_reg_rtx (V8QImode);
+  rtx v1 = gen_reg_rtx (V8QImode);
+  rtx r = gen_reg_rtx (QImode);
+  rtx in = operands[1];
+  rtx out = operands[0];
+  if(<MODE>mode == SImode)
+    {
+      rtx tmp;
+      tmp = gen_reg_rtx (DImode);
+      /* If we have SImode, zero extend to DImode, pop count does
+         not change if we have extra zeros. */
+      emit_insn (gen_zero_extendsidi2 (tmp, in));
+      in = tmp;
+    }
+  emit_move_insn (v, gen_lowpart (V8QImode, in));
+  emit_insn (gen_popcountv8qi2 (v1, v));
+  emit_insn (gen_reduc_plus_scal_v8qi (r, v1));
+  emit_insn (gen_zero_extendqi<mode>2 (out, r));
+  DONE;
+})
+
  (define_insn "clrsb<mode>2"
    [(set (match_operand:GPI 0 "register_operand" "=r")
          (clrsb:GPI (match_operand:GPI 1 "register_operand" "r")))]
diff --git a/gcc/testsuite/gcc.target/aarch64/popcount.c b/gcc/testsuite/gcc.target/aarch64/popcount.c
new file mode 100644
index 0000000..2d71168
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/popcount.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+int foo(int x)
+{
+  return __builtin_popcount(x);
+}
+
+/* { dg-final { scan-assembler-not "popcountdi2" } } */

__builtin_popcount takes an unsigned int, so this should be scanning for absence of popcountsi2 instead?
I suggest you also add a test for popcountdi2 using__builtin_popcountll.
Thanks,
Kyrill

+/* { dg-final { scan-assembler "cnt\t" } } */

Re: [PATCH] [AArch64] Implement popcount pattern

[Hurugalawadi, Naveen]

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

Hi Kyrill,

Thanks for reviewing the patch and your useful comments.

>> looks good to me if it has gone through the normal required
>> bootstrap and testing, but I can't approve.

Bootstrapped and Regression Tested on aarch64-thunderx-linux.

>> The rest of the MD file uses the term AdvSIMD. Also, the instrurction
>> is CNT rather than "pop count".

Done.

>> __builtin_popcount takes an unsigned int, so this should be 
>> scanning for absence of popcountsi2 instead?

Done.

Please find attached the modified patch as per review comments
and let me know if its okay for Stage-1 or current branch.

Thanks,
Naveen

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

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 65eb326..0acb3f0 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -3785,6 +3785,39 @@
   }
 )
 
+;; Pop count be done via the "CNT" instruction in AdvSIMD.
+;;
+;; MOV	v.1d, x0
+;; CNT	v1.8b, v.8b
+;; ADDV b2, v1.8b
+;; MOV	w0, v2.b[0]
+
+(define_expand "popcount<mode>2"
+  [(match_operand:GPI 0 "register_operand")
+   (match_operand:GPI 1 "register_operand")]
+  "TARGET_SIMD"
+{
+  rtx v = gen_reg_rtx (V8QImode);
+  rtx v1 = gen_reg_rtx (V8QImode);
+  rtx r = gen_reg_rtx (QImode);
+  rtx in = operands[1];
+  rtx out = operands[0];
+  if(<MODE>mode == SImode)
+    {
+      rtx tmp;
+      tmp = gen_reg_rtx (DImode);
+      /* If we have SImode, zero extend to DImode, pop count does
+         not change if we have extra zeros. */
+      emit_insn (gen_zero_extendsidi2 (tmp, in));
+      in = tmp;
+    }
+  emit_move_insn (v, gen_lowpart (V8QImode, in));
+  emit_insn (gen_popcountv8qi2 (v1, v));
+  emit_insn (gen_reduc_plus_scal_v8qi (r, v1));
+  emit_insn (gen_zero_extendqi<mode>2 (out, r));
+  DONE;
+})
+
 (define_insn "clrsb<mode>2"
   [(set (match_operand:GPI 0 "register_operand" "=r")
         (clrsb:GPI (match_operand:GPI 1 "register_operand" "r")))]
diff --git a/gcc/testsuite/gcc.target/aarch64/popcnt.c b/gcc/testsuite/gcc.target/aarch64/popcnt.c
new file mode 100644
index 0000000..37cf4b9
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/popcnt.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+int
+foo (int x)
+{
+  return __builtin_popcount (x);
+}
+
+long
+foo1 (int x)
+{
+  return __builtin_popcountl (x);
+}
+
+/* { dg-final { scan-assembler-not "popcount" } } */
+/* { dg-final { scan-assembler-times "cnt\t" 2 } } */

Re: [PATCH] [AArch64] Implement popcount pattern

[Kyrill Tkachov]

Hi Naveen,

On 13/12/16 11:51, Hurugalawadi, Naveen wrote:
> Hi Kyrill,
>
> Thanks for reviewing the patch and your useful comments.
>
>>> looks good to me if it has gone through the normal required
>>> bootstrap and testing, but I can't approve.
> Bootstrapped and Regression Tested on aarch64-thunderx-linux.
>
>>> The rest of the MD file uses the term AdvSIMD. Also, the instrurction
>>> is CNT rather than "pop count".
> Done.
>
>>> __builtin_popcount takes an unsigned int, so this should be
>>> scanning for absence of popcountsi2 instead?
> Done.
>
> Please find attached the modified patch as per review comments
> and let me know if its okay for Stage-1 or current branch.

This looks much better, thanks.
I still have a minor nit about the testcase.

+long
+foo1 (int x)
+{
+  return __builtin_popcountl (x);
+}

On ILP32 systems this would still use the SImode patterns, so I suggest you use __builtin_popcountll and
an unsigned long long return type to ensure you always exercise the 64-bit code.

+
+/* { dg-final { scan-assembler-not "popcount" } } */


This looks ok to me otherwise, but you'll need an ok from the aarch64 folk.
Kyrill

> Thanks,
> Naveen

[PATCH] [AArch64] Enable AES and cmp_branch fusion for Thunderx2t99

[Hurugalawadi, Naveen]

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

Hi,

Please find attached the patch that adds AES and CMP_BRANCH
fusion for Thunderx2t99.

Bootstrapped and Regression tested on aarch64-thunderx2t99.
Please review the patch and let us know if its okay?

2017-1-25  Naveen H.S <Naveen.Hurugalawadi@cavium.com>

gcc
        * config/aarch64/aarch64.c (thunderx2t99_tunings):
	Improve vector initialization code gen.

Thanks,
Naveen

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

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index f343d92..acaa975 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -780,7 +780,7 @@ static const struct tune_params thunderx2t99_tunings =
   &generic_approx_modes,
   4, /* memmov_cost.  */
   4, /* issue_rate.  */
-  AARCH64_FUSE_NOTHING, /* fuseable_ops.  */
+  (AARCH64_FUSE_CMP_BRANCH | AARCH64_FUSE_AES_AESMC), /* fusible_ops  */
   16,	/* function_align.  */
   8,	/* jump_align.  */
   16,	/* loop_align.  */

Re: [PATCH] [AArch64] Enable AES and cmp_branch fusion for Thunderx2t99

[Kyrill Tkachov]

Hi Naveen,

On 25/01/17 06:16, Hurugalawadi, Naveen wrote:
> Hi,
>
> Please find attached the patch that adds AES and CMP_BRANCH
> fusion for Thunderx2t99.
>
> Bootstrapped and Regression tested on aarch64-thunderx2t99.
> Please review the patch and let us know if its okay?

Code looks ok (it's quite simple), but I can't approve.
but there are a couple of issues with the ChangeLog

> 2017-1-25  Naveen H.S <Naveen.Hurugalawadi@cavium.com>

2017-01-25.
Also, two spaces between name and email

>
> gcc
>          * config/aarch64/aarch64.c (thunderx2t99_tunings):
> 	Improve vector initialization code gen.

This doesn't fit the code in the patch

Cheers,
Kyrill

Re: [PATCH] [AArch64] Implement popcount pattern

[James Greenhalgh]

On Tue, Dec 13, 2016 at 11:59:36AM +0000, Kyrill Tkachov wrote:
> Hi Naveen,
> 
> On 13/12/16 11:51, Hurugalawadi, Naveen wrote:
> >Hi Kyrill,
> >
> >Thanks for reviewing the patch and your useful comments.
> >
> >>>looks good to me if it has gone through the normal required
> >>>bootstrap and testing, but I can't approve.
> >Bootstrapped and Regression Tested on aarch64-thunderx-linux.
> >
> >>>The rest of the MD file uses the term AdvSIMD. Also, the instrurction
> >>>is CNT rather than "pop count".
> >Done.
> >
> >>>__builtin_popcount takes an unsigned int, so this should be
> >>>scanning for absence of popcountsi2 instead?
> >Done.
> >
> >Please find attached the modified patch as per review comments
> >and let me know if its okay for Stage-1 or current branch.
> 
> This looks much better, thanks.
> I still have a minor nit about the testcase.
> 
> +long
> +foo1 (int x)
> +{
> +  return __builtin_popcountl (x);
> +}
> 
> On ILP32 systems this would still use the SImode patterns, so I suggest you use __builtin_popcountll and
> an unsigned long long return type to ensure you always exercise the 64-bit code.
> 
> +
> +/* { dg-final { scan-assembler-not "popcount" } } */
> 
> 
> This looks ok to me otherwise, but you'll need an ok from the aarch64 folk.

I didn't see a follow-up patch posted with Kyrill's comments addressed?

Thanks,
James

Re: [PATCH] [AArch64] Implement popcount pattern

[Hurugalawadi, Naveen]

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

Hi James and Kyrill,

Thanks for the review and comments on the patch.

>> On ILP32 systems this would still use the SImode patterns, 
>> so I suggest you use __builtin_popcountll and
>> an unsigned long long return type to ensure you always exercise the 64-bit code.

Sorry for not commenting on this part.
The issue is that code generates "__popcountdi2" for all the codes in testcase
for LP64 and ILP32 variants.
__builtin_popcount, __builtin_popcountl and __builtin_popcount.

Hence, modified the patch to check for "popcount".

Bootstrapped and regression tested on AArch64-Thunderx-Linux machine.

Please find attached the modified patch and let us know if its okay?

Thanks,
Naveen

    

[-- Attachment #2: popcount-2.patch --]
[-- Type: application/octet-stream, Size: 1892 bytes --]

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index a693a3b..684a833 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -3778,6 +3778,39 @@
   }
 )
 
+;; Pop count be done via the "CNT" instruction in AdvSIMD.
+;;
+;; MOV	v.1d, x0
+;; CNT	v1.8b, v.8b
+;; ADDV b2, v1.8b
+;; MOV	w0, v2.b[0]
+
+(define_expand "popcount<mode>2"
+  [(match_operand:GPI 0 "register_operand")
+   (match_operand:GPI 1 "register_operand")]
+  "TARGET_SIMD"
+{
+  rtx v = gen_reg_rtx (V8QImode);
+  rtx v1 = gen_reg_rtx (V8QImode);
+  rtx r = gen_reg_rtx (QImode);
+  rtx in = operands[1];
+  rtx out = operands[0];
+  if(<MODE>mode == SImode)
+    {
+      rtx tmp;
+      tmp = gen_reg_rtx (DImode);
+      /* If we have SImode, zero extend to DImode, pop count does
+         not change if we have extra zeros. */
+      emit_insn (gen_zero_extendsidi2 (tmp, in));
+      in = tmp;
+    }
+  emit_move_insn (v, gen_lowpart (V8QImode, in));
+  emit_insn (gen_popcountv8qi2 (v1, v));
+  emit_insn (gen_reduc_plus_scal_v8qi (r, v1));
+  emit_insn (gen_zero_extendqi<mode>2 (out, r));
+  DONE;
+})
+
 (define_insn "clrsb<mode>2"
   [(set (match_operand:GPI 0 "register_operand" "=r")
         (clrsb:GPI (match_operand:GPI 1 "register_operand" "r")))]
diff --git a/gcc/testsuite/gcc.target/aarch64/popcnt.c b/gcc/testsuite/gcc.target/aarch64/popcnt.c
new file mode 100644
index 0000000..3b8cb31
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/popcnt.c
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+unsigned int
+foo (int x)
+{
+  return __builtin_popcount (x);
+}
+
+unsigned long
+foo1 (int x)
+{
+  return __builtin_popcountl (x);
+}
+
+unsigned long long
+foo2 (int x)
+{
+  return __builtin_popcountll (x);
+}
+
+/* { dg-final { scan-assembler-not "popcount" } } */
+/* { dg-final { scan-assembler-times "cnt\t" 3 } } */

Re: [PATCH] [AArch64] Enable AES and cmp_branch fusion for Thunderx2t99

[Hurugalawadi, Naveen]

Hi Kyrill,

Thanks for the review and comments.

>> but there are a couple of issues with the ChangeLog

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

	* config/aarch64/aarch64.c (thunderx2t99_tunings): Enable AES and
	cmp_branch fusion.

Thanks,
Naveen

Re: [PATCH] [AArch64] Enable AES and cmp_branch fusion for Thunderx2t99

[James Greenhalgh]

On Thu, Feb 02, 2017 at 05:03:13AM +0000, Hurugalawadi, Naveen wrote:
> Hi Kyrill,
> 
> Thanks for the review and comments.
> 
> >> but there are a couple of issues with the ChangeLog
> 
> 2017-02-02  Naveen H.S  <Naveen.Hurugalawadi@cavium.com>
> 
> 	* config/aarch64/aarch64.c (thunderx2t99_tunings): Enable AES and
> 	cmp_branch fusion.
> 

OK.

Thanks,
James

Re: [PATCH] [AArch64] Implement popcount pattern

[James Greenhalgh]

On Thu, Feb 02, 2017 at 04:03:42AM +0000, Hurugalawadi, Naveen wrote:
> Hi James and Kyrill,
> 
> Thanks for the review and comments on the patch.
> 
> >> On ILP32 systems this would still use the SImode patterns, 
> >> so I suggest you use __builtin_popcountll and
> >> an unsigned long long return type to ensure you always exercise the 64-bit code.
> 
> Sorry for not commenting on this part.
> The issue is that code generates "__popcountdi2" for all the codes in testcase
> for LP64 and ILP32 variants.
> __builtin_popcount, __builtin_popcountl and __builtin_popcount.
> 
> Hence, modified the patch to check for "popcount".

I don't understand this comment and how it relates to your updated
patch (which still checks for CNT).

> Bootstrapped and regression tested on AArch64-Thunderx-Linux machine.
> 
> Please find attached the modified patch and let us know if its okay?

Could you please clarify what you meant above? The patch looks fine (though
possibly not for Stage 4), but I'm not sure of your intent.

Thanks,
James

Re: [PATCH] [AArch64] Implement popcount pattern

[Andrew Pinski]

On Thu, Feb 2, 2017 at 3:55 AM, James Greenhalgh
<james.greenhalgh@arm.com> wrote:
> On Thu, Feb 02, 2017 at 04:03:42AM +0000, Hurugalawadi, Naveen wrote:
>> Hi James and Kyrill,
>>
>> Thanks for the review and comments on the patch.
>>
>> >> On ILP32 systems this would still use the SImode patterns,
>> >> so I suggest you use __builtin_popcountll and
>> >> an unsigned long long return type to ensure you always exercise the 64-bit code.
>>
>> Sorry for not commenting on this part.
>> The issue is that code generates "__popcountdi2" for all the codes in testcase
>> for LP64 and ILP32 variants.
>> __builtin_popcount, __builtin_popcountl and __builtin_popcount.
>>
>> Hence, modified the patch to check for "popcount".
>
> I don't understand this comment and how it relates to your updated
> patch (which still checks for CNT).

Now I understand Kyrill's comments as Naveen and I did not understand
before.  What Naveen was trying to test was that there was no longer
any call to __popcountdi2 or __popcountsi2 any more for all three:
__builtin_popcount, __builtin_popcountl and __builtin_popcountll.
Kyrill was asking him to change the test for __builtin_popcountl to
__builtin_popcountll even though there was already one test in that
file already.  Now of course what should change still is the argument
types to foo1/foo2 so they take long and long long respectively.  He
will post a new version of the patch soon.

>
>> Bootstrapped and regression tested on AArch64-Thunderx-Linux machine.
>>
>> Please find attached the modified patch and let us know if its okay?
>
> Could you please clarify what you meant above? The patch looks fine (though
> possibly not for Stage 4), but I'm not sure of your intent.

What he meant was he tested on aarch64-linux-gnu with no regressions.
Is it ok for when stage 1 opens with the change of the testcase?

Thanks,
Andrew

>
> Thanks,
> James

Re: [PATCH] [AArch64] Implement popcount pattern

[Hurugalawadi, Naveen]

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

Hi Andrew,

Thanks for clearing the confusion.

> I don't understand this comment and how it relates to your updated patch

foo, foo1 and foo2 generates calls to "popcountdi2" which should have
been "popcountsi2" for foo1. When Kyrill commented on using the
popcountsi2; I was confused :).

Hence, the testcase generally checks for the absence of call to "popcount"
and the presence of "cnt" instruction instead.

>> Now of course what should change still is the argument 
>> types to foo1/foo2 

The arguments to foo1 and foo2 are modified as required.

Bootstrapped and regression tested on aarch64-linux-gnu with no regressions.

Please let us know if its okay for stage 1?

Thanks,
Naveen

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

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index a693a3b..684a833 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -3778,6 +3778,39 @@
   }
 )
 
+;; Pop count be done via the "CNT" instruction in AdvSIMD.
+;;
+;; MOV	v.1d, x0
+;; CNT	v1.8b, v.8b
+;; ADDV b2, v1.8b
+;; MOV	w0, v2.b[0]
+
+(define_expand "popcount<mode>2"
+  [(match_operand:GPI 0 "register_operand")
+   (match_operand:GPI 1 "register_operand")]
+  "TARGET_SIMD"
+{
+  rtx v = gen_reg_rtx (V8QImode);
+  rtx v1 = gen_reg_rtx (V8QImode);
+  rtx r = gen_reg_rtx (QImode);
+  rtx in = operands[1];
+  rtx out = operands[0];
+  if(<MODE>mode == SImode)
+    {
+      rtx tmp;
+      tmp = gen_reg_rtx (DImode);
+      /* If we have SImode, zero extend to DImode, pop count does
+         not change if we have extra zeros. */
+      emit_insn (gen_zero_extendsidi2 (tmp, in));
+      in = tmp;
+    }
+  emit_move_insn (v, gen_lowpart (V8QImode, in));
+  emit_insn (gen_popcountv8qi2 (v1, v));
+  emit_insn (gen_reduc_plus_scal_v8qi (r, v1));
+  emit_insn (gen_zero_extendqi<mode>2 (out, r));
+  DONE;
+})
+
 (define_insn "clrsb<mode>2"
   [(set (match_operand:GPI 0 "register_operand" "=r")
         (clrsb:GPI (match_operand:GPI 1 "register_operand" "r")))]
diff --git a/gcc/testsuite/gcc.target/aarch64/popcnt.c b/gcc/testsuite/gcc.target/aarch64/popcnt.c
new file mode 100644
index 0000000..7e95796
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/popcnt.c
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+int
+foo (int x)
+{
+  return __builtin_popcount (x);
+}
+
+long
+foo1 (long x)
+{
+  return __builtin_popcountl (x);
+}
+
+long long
+foo2 (long long x)
+{
+  return __builtin_popcountll (x);
+}
+
+/* { dg-final { scan-assembler-not "popcount" } } */
+/* { dg-final { scan-assembler-times "cnt\t" 3 } } */

Re: [PATCH] [AArch64] Implement popcount pattern

[Kyrill Tkachov]

On 03/02/17 06:57, Hurugalawadi, Naveen wrote:
> Hi Andrew,
>
> Thanks for clearing the confusion.
>
>> I don't understand this comment and how it relates to your updated patch
> foo, foo1 and foo2 generates calls to "popcountdi2" which should have
> been "popcountsi2" for foo1. When Kyrill commented on using the
> popcountsi2; I was confused :).
>
> Hence, the testcase generally checks for the absence of call to "popcount"
> and the presence of "cnt" instruction instead.
>

Sorry for the confusion, I should have been more explicit that I was talking
about the optab.

>>> Now of course what should change still is the argument
>>> types to foo1/foo2
> The arguments to foo1 and foo2 are modified as required.
>
> Bootstrapped and regression tested on aarch64-linux-gnu with no regressions.
>
> Please let us know if its okay for stage 1?

This looks good to me, but you'll need James (or another aarch64 maintainer) to approve.

Thanks,
Kyrill

> Thanks,
> Naveen

Re: [PATCH] [AArch64] Implement popcount pattern

[James Greenhalgh]

On Fri, Feb 03, 2017 at 06:57:54AM +0000, Hurugalawadi, Naveen wrote:
> Hi Andrew,
> 
> Thanks for clearing the confusion.
> 
> > I don't understand this comment and how it relates to your updated patch
> 
> foo, foo1 and foo2 generates calls to "popcountdi2" which should have
> been "popcountsi2" for foo1. When Kyrill commented on using the
> popcountsi2; I was confused :).
> 
> Hence, the testcase generally checks for the absence of call to "popcount"
> and the presence of "cnt" instruction instead.
> 
> >> Now of course what should change still is the argument 
> >> types to foo1/foo2 
> 
> The arguments to foo1 and foo2 are modified as required.
> 
> Bootstrapped and regression tested on aarch64-linux-gnu with no regressions.
> 
> Please let us know if its okay for stage 1?

This looks OK to me now. The change is self contained, looks safe, and
has only been held up for the testcase issue.

But, please give Richard/Marcus a reasonable time to object (say, end of day
Tuesday) before committing.

Thanks,
James

> 
> Thanks,
> Naveen

> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index a693a3b..684a833 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -3778,6 +3778,39 @@
>    }
>  )
>  
> +;; Pop count be done via the "CNT" instruction in AdvSIMD.
> +;;
> +;; MOV	v.1d, x0
> +;; CNT	v1.8b, v.8b
> +;; ADDV b2, v1.8b
> +;; MOV	w0, v2.b[0]
> +
> +(define_expand "popcount<mode>2"
> +  [(match_operand:GPI 0 "register_operand")
> +   (match_operand:GPI 1 "register_operand")]
> +  "TARGET_SIMD"
> +{
> +  rtx v = gen_reg_rtx (V8QImode);
> +  rtx v1 = gen_reg_rtx (V8QImode);
> +  rtx r = gen_reg_rtx (QImode);
> +  rtx in = operands[1];
> +  rtx out = operands[0];
> +  if(<MODE>mode == SImode)
> +    {
> +      rtx tmp;
> +      tmp = gen_reg_rtx (DImode);
> +      /* If we have SImode, zero extend to DImode, pop count does
> +         not change if we have extra zeros. */
> +      emit_insn (gen_zero_extendsidi2 (tmp, in));
> +      in = tmp;
> +    }
> +  emit_move_insn (v, gen_lowpart (V8QImode, in));
> +  emit_insn (gen_popcountv8qi2 (v1, v));
> +  emit_insn (gen_reduc_plus_scal_v8qi (r, v1));
> +  emit_insn (gen_zero_extendqi<mode>2 (out, r));
> +  DONE;
> +})
> +
>  (define_insn "clrsb<mode>2"
>    [(set (match_operand:GPI 0 "register_operand" "=r")
>          (clrsb:GPI (match_operand:GPI 1 "register_operand" "r")))]
> diff --git a/gcc/testsuite/gcc.target/aarch64/popcnt.c b/gcc/testsuite/gcc.target/aarch64/popcnt.c
> new file mode 100644
> index 0000000..7e95796
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/popcnt.c
> @@ -0,0 +1,23 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +int
> +foo (int x)
> +{
> +  return __builtin_popcount (x);
> +}
> +
> +long
> +foo1 (long x)
> +{
> +  return __builtin_popcountl (x);
> +}
> +
> +long long
> +foo2 (long long x)
> +{
> +  return __builtin_popcountll (x);
> +}
> +
> +/* { dg-final { scan-assembler-not "popcount" } } */
> +/* { dg-final { scan-assembler-times "cnt\t" 3 } } */

Re: [PATCH] [AArch64] PR target/71663 Improve Vector Initializtion

[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/2016-12/msg00718.html

Thanks,
Naveen

[PING] [PATCH] [AArch64] PR target/71663 Improve Vector Initializtion

[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/2016-12/msg00718.html

Thanks,
Naveen




    

Re: [PATCH] [AArch64] PR target/71663 Improve Vector Initializtion

[Kyrill Tkachov]

Hi Naveen,

On 09/12/16 07:02, Hurugalawadi, Naveen wrote:
> Hi,
>
> Sorry. Missed out the testcase in patch submission.
> Added the missing testcase along with the ChangeLog.
> Please review the same and let us know if thats okay?

It would be useful if you expanded a bit on the approach used to
generate the improved codegen, or at least show for the testcase
what code was generated before this patch and what is generated
after this patch.

> 2016-12-09  Andrew PInski  <apinski@cavium.com>
>
> gcc
>          * config/aarch64/aarch64.c (aarch64_expand_vector_init):
>          Improve vector initialization code gen.
> gcc/testsuite
>          * gcc.target/aarch64/pr71663.c: New Testcase.

+  /* If there is only varables, try to optimize
+     the inseration using dup for the most common element
+     followed by insertations. */

Some typos: s/is only varables/are only variable elements/, s/inseration/insertion/,
s/insertations/insertions/.

  +  if (n_var == n_elts && n_elts <= 16)


diff --git a/gcc/testsuite/gcc.target/aarch64/pr71663.c b/gcc/testsuite/gcc.target/aarch64/pr71663.c
new file mode 100644
index 0000000..c8df847
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr71663.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#define vector __attribute__((vector_size(16)))
+
+vector float combine (float a, float b, float c, float d)
+{
+  return (vector float) { a, b, c, d };
+}

A large part of the aarch64.c hunk of your patch deals with finding the most commonly-occuring
element in the vector of variables, yet in your testcase all variables appear exactly once.
Perhaps worth adding a testcase where one of the vector elements appears more than the others?
I'd guess the codegen then would be better with this patch?

Cheers,
Kyrill

Re: [PATCH] [AArch64] PR target/71663 Improve Vector Initializtion

[Hurugalawadi, Naveen]

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

Hi Kyrill,

Thanks for the review and your comments.

>> It would be useful if you expanded a bit on the approach used to
>> generate the improved codegen

The patch creates a duplicate of most common element and tries to optimize
the insertion using dup for the element followed by insertions.

Current code:
============================================
        movi    v2.4s, 0
        ins     v2.s[0], v0.s[0]
        ins     v2.s[1], v1.s[0]
        ins     v2.s[2], v0.s[0]
        orr     v0.16b, v2.16b, v2.16b
        ins     v0.s[3], v3.s[0]
        ret
============================================

Code after the patch:
============================================
        dup     v0.4s, v0.s[0]
        ins     v0.s[1], v1.s[0]
        ins     v0.s[3], v3.s[0]
        ret
============================================

>> Some typos

Modified as required

>> worth adding a testcase where one of the vector elements appears more than
>> the others?

Modified the testcase as required using common element.

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

Thanks,
Naveen

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

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 2e385c4..8747a23 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -11671,11 +11671,54 @@ aarch64_expand_vector_init (rtx target, rtx vals)
       aarch64_expand_vector_init (target, copy);
     }
 
-  /* Insert the variable lanes directly.  */
-
   enum insn_code icode = optab_handler (vec_set_optab, mode);
   gcc_assert (icode != CODE_FOR_nothing);
 
+  /* If there are only variable elements, try to optimize
+     the insertion using dup for the most common element
+     followed by insertions.  */
+  if (n_var == n_elts && n_elts <= 16)
+    {
+      int matches[16][2];
+      int nummatches = 0;
+      memset (matches, 0, sizeof(matches));
+      for(int i = 0; i < n_elts; i++)
+	{
+	  for (int j = 0; j <= i; j++)
+	    {
+	      if (rtx_equal_p (XVECEXP (vals, 0, i), XVECEXP (vals, 0, j)))
+		{
+		  matches[i][0] = j;
+		  matches[j][1]++;
+		  if (i != j)
+		    nummatches++;
+		  break;
+		}
+	    }
+	}
+      int maxelement = 0;
+      int maxv = 0;
+      for (int i = 0; i < n_elts; i++)
+	if (matches[i][1] > maxv)
+	  maxelement = i, maxv = matches[i][1];
+
+      /* Create a duplicate of the most common element.  */
+      rtx x = copy_to_mode_reg (inner_mode, XVECEXP (vals, 0, maxelement));
+      aarch64_emit_move (target, gen_rtx_VEC_DUPLICATE (mode, x));
+
+      /* Insert the rest.  */
+      for (int i = 0; i < n_elts; i++)
+	{
+	  rtx x = XVECEXP (vals, 0, i);
+	  if (matches[i][0] == maxelement)
+	    continue;
+	  x = copy_to_mode_reg (inner_mode, x);
+	  emit_insn (GEN_FCN (icode) (target, x, GEN_INT (i)));
+	}
+      return;
+    }
+
+  /* Insert the variable lanes directly.  */
   for (int i = 0; i < n_elts; i++)
     {
       rtx x = XVECEXP (vals, 0, i);
diff --git a/gcc/testsuite/gcc.target/aarch64/pr71663.c b/gcc/testsuite/gcc.target/aarch64/pr71663.c
new file mode 100644
index 0000000..a043a21
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr71663.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#define vector __attribute__((vector_size(16)))
+
+vector float combine (float a, float b, float c, float d)
+{
+  return (vector float) { a, b, a, d };
+}
+
+/* { dg-final { scan-assembler-not "movi\t" } } */
+/* { dg-final { scan-assembler-not "orr\t" } } */
+/* { dg-final { scan-assembler-times "ins\t" 2 } } */
+/* { dg-final { scan-assembler-times "dup\t" 1 } } */

Re: [PING] [PATCH] [AArch64] PR target/71663 Improve Vector Initializtion

[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/msg01260.html

Thanks,
Naveen

Re: [PING 2] [PATCH] [AArch64] PR target/71663 Improve Vector Initializtion

[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/msg01260.html

Thanks,
Naveen



    

Re: [PATCH] [AArch64] PR target/71663 Improve Vector Initializtion

[James Greenhalgh]

On Wed, Apr 26, 2017 at 06:34:36AM +0000, Hurugalawadi, Naveen wrote:
> Hi Kyrill,
> 
> Thanks for the review and your comments.
> 
> >> It would be useful if you expanded a bit on the approach used to
> >> generate the improved codegen
> 
> The patch creates a duplicate of most common element and tries to optimize
> the insertion using dup for the element followed by insertions.
> 
> Current code:
> ============================================
>         movi    v2.4s, 0
>         ins     v2.s[0], v0.s[0]
>         ins     v2.s[1], v1.s[0]
>         ins     v2.s[2], v0.s[0]
>         orr     v0.16b, v2.16b, v2.16b
>         ins     v0.s[3], v3.s[0]
>         ret
> ============================================
> 
> Code after the patch:
> ============================================
>         dup     v0.4s, v0.s[0]
>         ins     v0.s[1], v1.s[0]
>         ins     v0.s[3], v3.s[0]
>         ret
> ============================================
> 
> >> Some typos
> 
> Modified as required
> 
> >> worth adding a testcase where one of the vector elements appears more than
> >> the others?
> 
> Modified the testcase as required using common element.
> 
> Please review the patch and let us know if its okay?
> Bootstrapped and Regression tested on aarch64-thunder-linux.

This patch fell through the cracks as it shows up in a reply chain with a
few others. If you could try to keep one reply chain for each patch series
you're submitting, that would make tracking the submissions much
easier :-).


> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 2e385c4..8747a23 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -11671,11 +11671,54 @@ aarch64_expand_vector_init (rtx target, rtx vals)
>        aarch64_expand_vector_init (target, copy);
>      }
>  
> -  /* Insert the variable lanes directly.  */
> -
>    enum insn_code icode = optab_handler (vec_set_optab, mode);
>    gcc_assert (icode != CODE_FOR_nothing);
>  
> +  /* If there are only variable elements, try to optimize
> +     the insertion using dup for the most common element
> +     followed by insertions.  */
> +  if (n_var == n_elts && n_elts <= 16)
> +    {
> +      int matches[16][2];
> +      int nummatches = 0;
> +      memset (matches, 0, sizeof(matches));

Very minor, but what is wrong with:

  int matches[16][2] = {0};

Rather than the explicit memset?

> +      for(int i = 0; i < n_elts; i++)
> +	{
> +	  for (int j = 0; j <= i; j++)
> +	    {
> +	      if (rtx_equal_p (XVECEXP (vals, 0, i), XVECEXP (vals, 0, j)))
> +		{
> +		  matches[i][0] = j;
> +		  matches[j][1]++;
> +		  if (i != j)
> +		    nummatches++;

nummatches is unused.

This search algorithm is tough to follow. A comment explaining that you will
fill matches[*][0] with the earliest matching element, and matches[X][1]
with the count of duplicate elements (if X is the earliest element which has
duplicates). Would be useful to understand exactly what you're aiming for.
Certainly it took me a while to understand that for:

  { a, b, c, b, b, d, c, e }

matches would look like:

  { { 0, 1 },
    { 1, 3 },
    { 2. 2 },
    { 1, 0 },
    { 1, 0 },
    { 5, 1 },
    { 2, 0 },
    { 7, 1 } }

> +		}
> +	    }
> +	}
> +      int maxelement = 0;
> +      int maxv = 0;
> +      for (int i = 0; i < n_elts; i++)
> +	if (matches[i][1] > maxv)
> +	  maxelement = i, maxv = matches[i][1];

Put braces round this and write it as two statements, or eliminate the use of
maxv and use matches[i][1] > matches[maxelement][1], but don't use the comma
operator like this please.

> +      /* Create a duplicate of the most common element.  */
> +      rtx x = copy_to_mode_reg (inner_mode, XVECEXP (vals, 0, maxelement));
> +      aarch64_emit_move (target, gen_rtx_VEC_DUPLICATE (mode, x));
> +
> +      /* Insert the rest.  */
> +      for (int i = 0; i < n_elts; i++)
> +	{
> +	  rtx x = XVECEXP (vals, 0, i);
> +	  if (matches[i][0] == maxelement)
> +	    continue;
> +	  x = copy_to_mode_reg (inner_mode, x);
> +	  emit_insn (GEN_FCN (icode) (target, x, GEN_INT (i)));
> +	}
> +      return;
> +    }
> +
> +  /* Insert the variable lanes directly.  */

This code would read better if you rearranged the cases. As it stands your
code is added in the middle of a logical operation (deal with constant lanes,
then deal with variable lanes). Move your new code above the part-variable
case.

>    for (int i = 0; i < n_elts; i++)
>      {
>        rtx x = XVECEXP (vals, 0, i);
> diff --git a/gcc/testsuite/gcc.target/aarch64/pr71663.c b/gcc/testsuite/gcc.target/aarch64/pr71663.c
> new file mode 100644
> index 0000000..a043a21
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/pr71663.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +#define vector __attribute__((vector_size(16)))
> +
> +vector float combine (float a, float b, float c, float d)

c is unused.

> +{
> +  return (vector float) { a, b, a, d };
> +}
> +
> +/* { dg-final { scan-assembler-not "movi\t" } } */
> +/* { dg-final { scan-assembler-not "orr\t" } } */
> +/* { dg-final { scan-assembler-times "ins\t" 2 } } */
> +/* { dg-final { scan-assembler-times "dup\t" 1 } } */

Re: [PATCH] [AArch64] PR target/71663 Improve Vector Initializtion

[Hurugalawadi, Naveen]

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

Hi James,

Thanks for your review and useful comments.

>> If you could try to keep one reply chain for each patch series
Will keep that in mind for sure :-)

>> Very minor, but what is wrong with:
>> int matches[16][2] = {0};
Done.

>> nummatches is unused.
Removed.

>> This search algorithm is tough to follow
Updated as per your comments.

>> Put braces round this and write it as two statements
Done.

>> Move your new code above the part-variable case.
Done.

>> c is unused.
Removed.

Bootstrapped and Regression tested on aarch64-thunder-linux.

Please review the patch and let us know if any comments or suggestions.

Thanks,
Naveen
    

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

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index bce490f..239ba72 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -11707,6 +11707,57 @@ aarch64_expand_vector_init (rtx target, rtx vals)
       return;
     }
 
+  enum insn_code icode = optab_handler (vec_set_optab, mode);
+  gcc_assert (icode != CODE_FOR_nothing);
+
+  /* If there are only variable elements, try to optimize
+     the insertion using dup for the most common element
+     followed by insertions.  */
+
+  /* The algorithm will fill matches[*][0] with the earliest matching element,
+     and matches[X][1] with the count of duplicate elements (if X is the
+     earliest element which has duplicates).  */
+
+  if (n_var == n_elts && n_elts <= 16)
+    {
+      int matches[16][2] = {0};
+      for (int i = 0; i < n_elts; i++)
+	{
+	  for (int j = 0; j <= i; j++)
+	    {
+	      if (rtx_equal_p (XVECEXP (vals, 0, i), XVECEXP (vals, 0, j)))
+		{
+		  matches[i][0] = j;
+		  matches[j][1]++;
+		  break;
+		}
+	    }
+	}
+      int maxelement = 0;
+      int maxv = 0;
+      for (int i = 0; i < n_elts; i++)
+	if (matches[i][1] > maxv)
+	  {
+	    maxelement = i;
+	    maxv = matches[i][1];
+	  }
+
+      /* Create a duplicate of the most common element.  */
+      rtx x = copy_to_mode_reg (inner_mode, XVECEXP (vals, 0, maxelement));
+      aarch64_emit_move (target, gen_rtx_VEC_DUPLICATE (mode, x));
+
+      /* Insert the rest.  */
+      for (int i = 0; i < n_elts; i++)
+	{
+	  rtx x = XVECEXP (vals, 0, i);
+	  if (matches[i][0] == maxelement)
+	    continue;
+	  x = copy_to_mode_reg (inner_mode, x);
+	  emit_insn (GEN_FCN (icode) (target, x, GEN_INT (i)));
+	}
+      return;
+    }
+
   /* Initialise a vector which is part-variable.  We want to first try
      to build those lanes which are constant in the most efficient way we
      can.  */
@@ -11740,10 +11791,6 @@ aarch64_expand_vector_init (rtx target, rtx vals)
     }
 
   /* Insert the variable lanes directly.  */
-
-  enum insn_code icode = optab_handler (vec_set_optab, mode);
-  gcc_assert (icode != CODE_FOR_nothing);
-
   for (int i = 0; i < n_elts; i++)
     {
       rtx x = XVECEXP (vals, 0, i);
diff --git a/gcc/testsuite/gcc.target/aarch64/pr71663.c b/gcc/testsuite/gcc.target/aarch64/pr71663.c
new file mode 100644
index 0000000..65f368d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr71663.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#define vector __attribute__((vector_size(16)))
+
+vector float combine (float a, float b, float d)
+{
+  return (vector float) { a, b, a, d };
+}
+
+/* { dg-final { scan-assembler-not "movi\t" } } */
+/* { dg-final { scan-assembler-not "orr\t" } } */
+/* { dg-final { scan-assembler-times "ins\t" 2 } } */
+/* { dg-final { scan-assembler-times "dup\t" 1 } } */

Re: [PATCH] [AArch64] PR target/71663 Improve Vector Initializtion

[James Greenhalgh]

On Tue, Jun 13, 2017 at 10:24:59AM +0000, Hurugalawadi, Naveen wrote:
> Hi James,
> 
> Thanks for your review and useful comments.
> 
> >> If you could try to keep one reply chain for each patch series
> Will keep that in mind for sure :-)
> 
> >> Very minor, but what is wrong with:
> >> int matches[16][2] = {0};
> Done.
> 
> >> nummatches is unused.
> Removed.
> 
> >> This search algorithm is tough to follow
> Updated as per your comments.
> 
> >> Put braces round this and write it as two statements
> Done.
> 
> >> Move your new code above the part-variable case.
> Done.
> 
> >> c is unused.
> Removed.
> 
> Bootstrapped and Regression tested on aarch64-thunder-linux.
> 
> Please review the patch and let us know if any comments or suggestions.

Almost OK. Could you make the testcase a bit more comprehensive? Test
that the right thing happens with multiple duplicates, that the right thing
happens when there are no duplicates, etc. At the moment the test does not
provide good coverage of the cases your code handles.

With a fuller testcase this will likely be OK, but please repost the patch
for another review.

Thanks,
James

> diff --git a/gcc/testsuite/gcc.target/aarch64/pr71663.c b/gcc/testsuite/gcc.target/aarch64/pr71663.c
> new file mode 100644
> index 0000000..65f368d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/pr71663.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +#define vector __attribute__((vector_size(16)))
> +
> +vector float combine (float a, float b, float d)
> +{
> +  return (vector float) { a, b, a, d };
> +}
> +
> +/* { dg-final { scan-assembler-not "movi\t" } } */
> +/* { dg-final { scan-assembler-not "orr\t" } } */
> +/* { dg-final { scan-assembler-times "ins\t" 2 } } */
> +/* { dg-final { scan-assembler-times "dup\t" 1 } } */

Re: [PATCH] [AArch64] PR target/71663 Improve Vector Initializtion

[Hurugalawadi, Naveen]

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

Hi James,

>> Could you make the testcase a bit more comprehensive? 

Modified the testcase considering all the possible cases.
Split up the test based on different scenarios.

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

Thanks,
Naveen

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

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index bce490f..239ba72 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -11707,6 +11707,57 @@ aarch64_expand_vector_init (rtx target, rtx vals)
       return;
     }
 
+  enum insn_code icode = optab_handler (vec_set_optab, mode);
+  gcc_assert (icode != CODE_FOR_nothing);
+
+  /* If there are only variable elements, try to optimize
+     the insertion using dup for the most common element
+     followed by insertions.  */
+
+  /* The algorithm will fill matches[*][0] with the earliest matching element,
+     and matches[X][1] with the count of duplicate elements (if X is the
+     earliest element which has duplicates).  */
+
+  if (n_var == n_elts && n_elts <= 16)
+    {
+      int matches[16][2] = {0};
+      for (int i = 0; i < n_elts; i++)
+	{
+	  for (int j = 0; j <= i; j++)
+	    {
+	      if (rtx_equal_p (XVECEXP (vals, 0, i), XVECEXP (vals, 0, j)))
+		{
+		  matches[i][0] = j;
+		  matches[j][1]++;
+		  break;
+		}
+	    }
+	}
+      int maxelement = 0;
+      int maxv = 0;
+      for (int i = 0; i < n_elts; i++)
+	if (matches[i][1] > maxv)
+	  {
+	    maxelement = i;
+	    maxv = matches[i][1];
+	  }
+
+      /* Create a duplicate of the most common element.  */
+      rtx x = copy_to_mode_reg (inner_mode, XVECEXP (vals, 0, maxelement));
+      aarch64_emit_move (target, gen_rtx_VEC_DUPLICATE (mode, x));
+
+      /* Insert the rest.  */
+      for (int i = 0; i < n_elts; i++)
+	{
+	  rtx x = XVECEXP (vals, 0, i);
+	  if (matches[i][0] == maxelement)
+	    continue;
+	  x = copy_to_mode_reg (inner_mode, x);
+	  emit_insn (GEN_FCN (icode) (target, x, GEN_INT (i)));
+	}
+      return;
+    }
+
   /* Initialise a vector which is part-variable.  We want to first try
      to build those lanes which are constant in the most efficient way we
      can.  */
@@ -11740,10 +11791,6 @@ aarch64_expand_vector_init (rtx target, rtx vals)
     }
 
   /* Insert the variable lanes directly.  */
-
-  enum insn_code icode = optab_handler (vec_set_optab, mode);
-  gcc_assert (icode != CODE_FOR_nothing);
-
   for (int i = 0; i < n_elts; i++)
     {
       rtx x = XVECEXP (vals, 0, i);
diff --git a/gcc/testsuite/gcc.target/aarch64/vect-init-1.c b/gcc/testsuite/gcc.target/aarch64/vect-init-1.c
new file mode 100644
index 0000000..90ba3ae
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/vect-init-1.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#define vector __attribute__((vector_size(16)))
+
+vector float combine (float a, float b, float c, float d)
+{
+  return (vector float) { a, b, c, d };
+}
+
+/* { dg-final { scan-assembler-not "movi\t" } } */
+/* { dg-final { scan-assembler-not "orr\t" } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/vect-init-2.c b/gcc/testsuite/gcc.target/aarch64/vect-init-2.c
new file mode 100644
index 0000000..0444675
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/vect-init-2.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#define vector __attribute__((vector_size(16)))
+
+vector float combine (float a, float b, float d)
+{
+  return (vector float) { a, b, a, d };
+}
+
+/* { dg-final { scan-assembler-not "movi\t" } } */
+/* { dg-final { scan-assembler-not "orr\t" } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/vect-init-3.c b/gcc/testsuite/gcc.target/aarch64/vect-init-3.c
new file mode 100644
index 0000000..b5822b7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/vect-init-3.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#define vector __attribute__((vector_size(16)))
+
+vector float combine (float a, float b)
+{
+  return (vector float) { a, b, a, b };
+}
+
+/* { dg-final { scan-assembler-not "movi\t" } } */
+/* { dg-final { scan-assembler-not "orr\t" } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/vect-init-4.c b/gcc/testsuite/gcc.target/aarch64/vect-init-4.c
new file mode 100644
index 0000000..09a0095
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/vect-init-4.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#define vector __attribute__((vector_size(16)))
+
+vector float combine (float a, float b)
+{
+  return (vector float) { a, b, b, a };
+}
+
+/* { dg-final { scan-assembler-not "movi\t" } } */
+/* { dg-final { scan-assembler-not "orr\t" } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/vect-init-5.c b/gcc/testsuite/gcc.target/aarch64/vect-init-5.c
new file mode 100644
index 0000000..76d5502
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/vect-init-5.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#define vector __attribute__((vector_size(16)))
+
+vector float combine (float a, float b)
+{
+  return (vector float) { a, b, a, a };
+}
+
+/* { dg-final { scan-assembler-not "movi\t" } } */
+/* { dg-final { scan-assembler-not "orr\t" } } */

Re: [PATCH] [AArch64] PR target/71663 Improve Vector Initializtion

[James Greenhalgh]

On Wed, Jun 14, 2017 at 08:53:27AM +0000, Hurugalawadi, Naveen wrote:
> Hi James,
> 
> >> Could you make the testcase a bit more comprehensive? 
> 
> Modified the testcase considering all the possible cases.
> Split up the test based on different scenarios.
> 
> Please review the patch and let us know if its okay?

OK with an appropriate ChangeLog. Thanks for the patch!

Thanks,
James