Re: [PATCH AArch64]Support missing vcond pattern by adding/using vec_cmp/vcond_mask patterns.

[James Greenhalgh]

On Wed, Jun 08, 2016 at 03:57:42PM +0100, Bin Cheng wrote:
> > From: James Greenhalgh <james.greenha...@arm.com>
> > Sent: 31 May 2016 16:24
> > To: Bin Cheng
> > Cc: gcc-patches@gcc.gnu.org; nd
> > Subject: Re: [PATCH AArch64]Support missing vcond pattern by adding/using 
> > vec_cmp/vcond_mask patterns.
> > 
> > On Tue, May 17, 2016 at 09:02:22AM +, Bin Cheng wrote:
> > > Hi,
> > > Alan and Renlin noticed that some vcond patterns are not supported in
> > > AArch64(or AArch32?) backend, and they both had some patches fixing this.
> > > After investigation, I agree with them that vcond/vcondu in AArch64's 
> > > backend
> > > should be re-implemented using vec_cmp/vcond_mask patterns, so here comes
> > > this patch which is based on Alan's.  This patch supports all vcond/vcondu
> > > patterns by implementing/using vec_cmp and vcond_mask patterns.  
> > > Different to
> > > the original patch, it doesn't change GCC's expanding process, and it 
> > > keeps
> > > vcond patterns.  The patch also introduces vec_cmp*_internal to support
> > > special case optimization for vcond/vcondu which current implementation 
> > > does.
> > > Apart from Alan's patch, I also learned ideas from Renlin's, and it is my
> > > change that shall be blamed if any potential bug is introduced.
> > > 
> > > With this patch, GCC's test condition "vect_cond_mixed" can be enabled on
> > > AArch64 (in a following patch).  Bootstrap and test on AArch64.  Is it OK?
> > > BTW, this patch is necessary for gcc.dg/vect/PR56541.c (on AArch64) which 
> > > was
> > > added before in tree if-conversion patch.
> > 
> > Splitting this patch would have been very helpful. One patch each for the
> > new standard pattern names, and one patch for the refactor of vcond. As
> > it is, this patch is rather difficult to read.
> Done, patch split into two with one implementing new vcond_mask_cmp
> patterns, and another re-writing vcond patterns.
> 

Hi Bin,

Thanks for the changes. Just to make keeping track of review easier for
me, would you mind resending these in series as two seperate emails.

i.e.

[Patch AArch64 1/2]  Implement vcond_mask and vec_cmp patterns
[Patch AArch64 2/2]  Rewrite vcond patterns

Thanks,
James

Re: [PATCH AArch64]Support missing vcond pattern by adding/using vec_cmp/vcond_mask patterns.

[Bin Cheng]

> From: James Greenhalgh <james.greenha...@arm.com>
> Sent: 31 May 2016 16:24
> To: Bin Cheng
> Cc: gcc-patches@gcc.gnu.org; nd
> Subject: Re: [PATCH AArch64]Support missing vcond pattern by adding/using 
> vec_cmp/vcond_mask patterns.
> 
> On Tue, May 17, 2016 at 09:02:22AM +, Bin Cheng wrote:
> > Hi,
> > Alan and Renlin noticed that some vcond patterns are not supported in
> > AArch64(or AArch32?) backend, and they both had some patches fixing this.
> > After investigation, I agree with them that vcond/vcondu in AArch64's 
> > backend
> > should be re-implemented using vec_cmp/vcond_mask patterns, so here comes
> > this patch which is based on Alan's.  This patch supports all vcond/vcondu
> > patterns by implementing/using vec_cmp and vcond_mask patterns.  Different 
> > to
> > the original patch, it doesn't change GCC's expanding process, and it keeps
> > vcond patterns.  The patch also introduces vec_cmp*_internal to support
> > special case optimization for vcond/vcondu which current implementation 
> > does.
> > Apart from Alan's patch, I also learned ideas from Renlin's, and it is my
> > change that shall be blamed if any potential bug is introduced.
> > 
> > With this patch, GCC's test condition "vect_cond_mixed" can be enabled on
> > AArch64 (in a following patch).  Bootstrap and test on AArch64.  Is it OK?
> > BTW, this patch is necessary for gcc.dg/vect/PR56541.c (on AArch64) which 
> > was
> > added before in tree if-conversion patch.
> 
> Splitting this patch would have been very helpful. One patch each for the
> new standard pattern names, and one patch for the refactor of vcond. As
> it is, this patch is rather difficult to read.
Done, patch split into two with one implementing new vcond_mask_cmp 
patterns, and another re-writing vcond patterns.

> 
> > diff --git a/gcc/config/aarch64/aarch64-simd.md 
> > b/gcc/config/aarch64/aarch64-simd.md
> > index bd73bce..f51473a 100644
> > --- a/gcc/config/aarch64/aarch64-simd.md
> > +++ b/gcc/config/aarch64/aarch64-simd.md
> > @@ -1053,7 +1053,7 @@
> >  }
> >  
> >cmp_fmt = gen_rtx_fmt_ee (cmp_operator, V2DImode, operands[1], 
> > operands[2]);
> > -  emit_insn (gen_aarch64_vcond_internalv2div2di (operands[0], operands[1],
> > +  emit_insn (gen_vcondv2div2di (operands[0], operands[1],
> >operands[2], cmp_fmt, operands[1], operands[2]));
> >DONE;
> >  })
> > @@ -2225,204 +2225,215 @@
> >DONE;
> >  })
> >  
> > -(define_expand "aarch64_vcond_internal"
> > +(define_expand "vcond_mask_"
> > +  [(match_operand:VALLDI 0 "register_operand")
> > +   (match_operand:VALLDI 1 "nonmemory_operand")
> > +   (match_operand:VALLDI 2 "nonmemory_operand")
> > +   (match_operand: 3 "register_operand")]
> > +  "TARGET_SIMD"
> > +{
> > +  /* If we have (a = (P) ? -1 : 0);
> > + Then we can simply move the generated mask (result must be int).  */
> > +  if (operands[1] == CONSTM1_RTX (mode)
> > +  && operands[2] == CONST0_RTX (mode))
> > +emit_move_insn (operands[0], operands[3]);
> > +  /* Similarly, (a = (P) ? 0 : -1) is just inverting the generated mask.  
> > */
> > +  else if (operands[1] == CONST0_RTX (mode)
> > +&& operands[2] == CONSTM1_RTX (mode))
> > +emit_insn (gen_one_cmpl2 (operands[0], operands[3]));
> > +  else
> > +{
> > +  if (!REG_P (operands[1]))
> > + operands[1] = force_reg (mode, operands[1]);
> > +  if (!REG_P (operands[2]))
> > + operands[2] = force_reg (mode, operands[2]);
> > +  emit_insn (gen_aarch64_simd_bsl (operands[0], operands[3],
> > +  operands[1], operands[2]));
> > +}
> > +
> > +  DONE;
> > +})
> > +
> 
> This pattern is fine.
> 
> > +;; Patterns comparing two vectors to produce a mask.
> 
> This comment is insufficient. The logic in vec_cmp_internal
> does not always return the expected mask (in particular for NE), but this
> is not made clear in the comment.
Comments added to various places to make the code easier to understand and 
maintain.

> 
> > +
> > +(define_expand "vec_cmp_internal"
> >[(set (match_operand:VSDQ_I_DI 0 "register_operand")
> > - (if_then_else:VSDQ_I_DI
> > -   (match_operator 3 "comparison_operator"
> > - [(match_operand:VSDQ_I_DI 4 "register_operand")
> > -  (match_operand:VSDQ_I_DI 5 &q

Re: [PATCH AArch64]Support missing vcond pattern by adding/using vec_cmp/vcond_mask patterns.

[James Greenhalgh]

On Tue, May 17, 2016 at 09:02:22AM +, Bin Cheng wrote:
> Hi,
> Alan and Renlin noticed that some vcond patterns are not supported in
> AArch64(or AArch32?) backend, and they both had some patches fixing this.
> After investigation, I agree with them that vcond/vcondu in AArch64's backend
> should be re-implemented using vec_cmp/vcond_mask patterns, so here comes
> this patch which is based on Alan's.  This patch supports all vcond/vcondu
> patterns by implementing/using vec_cmp and vcond_mask patterns.  Different to
> the original patch, it doesn't change GCC's expanding process, and it keeps
> vcond patterns.  The patch also introduces vec_cmp*_internal to support
> special case optimization for vcond/vcondu which current implementation does.
> Apart from Alan's patch, I also learned ideas from Renlin's, and it is my
> change that shall be blamed if any potential bug is introduced.
> 
> With this patch, GCC's test condition "vect_cond_mixed" can be enabled on
> AArch64 (in a following patch).  Bootstrap and test on AArch64.  Is it OK?
> BTW, this patch is necessary for gcc.dg/vect/PR56541.c (on AArch64) which was
> added before in tree if-conversion patch.

Splitting this patch would have been very helpful. One patch each for the
new standard pattern names, and one patch for the refactor of vcond. As
it is, this patch is rather difficult to read.

> diff --git a/gcc/config/aarch64/aarch64-simd.md 
> b/gcc/config/aarch64/aarch64-simd.md
> index bd73bce..f51473a 100644
> --- a/gcc/config/aarch64/aarch64-simd.md
> +++ b/gcc/config/aarch64/aarch64-simd.md
> @@ -1053,7 +1053,7 @@
>  }
>  
>cmp_fmt = gen_rtx_fmt_ee (cmp_operator, V2DImode, operands[1], 
> operands[2]);
> -  emit_insn (gen_aarch64_vcond_internalv2div2di (operands[0], operands[1],
> +  emit_insn (gen_vcondv2div2di (operands[0], operands[1],
>operands[2], cmp_fmt, operands[1], operands[2]));
>DONE;
>  })
> @@ -2225,204 +2225,215 @@
>DONE;
>  })
>  
> -(define_expand "aarch64_vcond_internal"
> +(define_expand "vcond_mask_"
> +  [(match_operand:VALLDI 0 "register_operand")
> +   (match_operand:VALLDI 1 "nonmemory_operand")
> +   (match_operand:VALLDI 2 "nonmemory_operand")
> +   (match_operand: 3 "register_operand")]
> +  "TARGET_SIMD"
> +{
> +  /* If we have (a = (P) ? -1 : 0);
> + Then we can simply move the generated mask (result must be int).  */
> +  if (operands[1] == CONSTM1_RTX (mode)
> +  && operands[2] == CONST0_RTX (mode))
> +emit_move_insn (operands[0], operands[3]);
> +  /* Similarly, (a = (P) ? 0 : -1) is just inverting the generated mask.  */
> +  else if (operands[1] == CONST0_RTX (mode)
> +&& operands[2] == CONSTM1_RTX (mode))
> +emit_insn (gen_one_cmpl2 (operands[0], operands[3]));
> +  else
> +{
> +  if (!REG_P (operands[1]))
> + operands[1] = force_reg (mode, operands[1]);
> +  if (!REG_P (operands[2]))
> + operands[2] = force_reg (mode, operands[2]);
> +  emit_insn (gen_aarch64_simd_bsl (operands[0], operands[3],
> +  operands[1], operands[2]));
> +}
> +
> +  DONE;
> +})
> +

This pattern is fine.

> +;; Patterns comparing two vectors to produce a mask.

This comment is insufficient. The logic in vec_cmp_internal
does not always return the expected mask (in particular for NE), but this
is not made clear in the comment.

> +
> +(define_expand "vec_cmp_internal"
>[(set (match_operand:VSDQ_I_DI 0 "register_operand")
> - (if_then_else:VSDQ_I_DI
> -   (match_operator 3 "comparison_operator"
> - [(match_operand:VSDQ_I_DI 4 "register_operand")
> -  (match_operand:VSDQ_I_DI 5 "nonmemory_operand")])
> -   (match_operand:VSDQ_I_DI 1 "nonmemory_operand")
> -   (match_operand:VSDQ_I_DI 2 "nonmemory_operand")))]
> +   (match_operator 1 "comparison_operator"
> + [(match_operand:VSDQ_I_DI 2 "register_operand")
> +  (match_operand:VSDQ_I_DI 3 "nonmemory_operand")]))]
>"TARGET_SIMD"



> +(define_expand "vec_cmp"
> +  [(set (match_operand:VSDQ_I_DI 0 "register_operand")
> +   (match_operator 1 "comparison_operator"
> + [(match_operand:VSDQ_I_DI 2 "register_operand")
> +  (match_operand:VSDQ_I_DI 3 "nonmemory_operand")]))]
> +  "TARGET_SIMD"
> +{
> +  enum rtx_code code = GET_CODE (operands[1]);
> +
> +  emit_insn (gen_vec_cmp_internal (operands[0],
> +  operands[1], operands[2],
> +  operands[3]));
> +  /* See comments in vec_cmp_internal, below
> + operators are computed using other operators, and we need to
> + revert the result.  */

s/revert/invert

There are no comments in vec_cmp_internal for this
to refer to. But, there should be - more comments would definitely help
this patch!

These comments apply equally to the other copies of this comment in the
patch.

> +  if (code == NE)
> +emit_insn (gen_one_cmpl2 (operands[0], operands[0]));
>DONE;
>  })

Re: [PATCH AArch64]Support missing vcond pattern by adding/using vec_cmp/vcond_mask patterns.

[Bin.Cheng]

Ping.

Thanks,
bin

On Tue, May 17, 2016 at 10:02 AM, Bin Cheng  wrote:
> Hi,
> Alan and Renlin noticed that some vcond patterns are not supported in 
> AArch64(or AArch32?) backend, and they both had some patches fixing this.  
> After investigation, I agree with them that vcond/vcondu in AArch64's backend 
> should be re-implemented using vec_cmp/vcond_mask patterns, so here comes 
> this patch which is based on Alan's.  This patch supports all vcond/vcondu 
> patterns by implementing/using vec_cmp and vcond_mask patterns.  Different to 
> the original patch, it doesn't change GCC's expanding process, and it keeps 
> vcond patterns.  The patch also introduces vec_cmp*_internal to support 
> special case optimization for vcond/vcondu which current implementation does.
> Apart from Alan's patch, I also learned ideas from Renlin's, and it is my 
> change that shall be blamed if any potential bug is introduced.
>
> With this patch, GCC's test condition "vect_cond_mixed" can be enabled on 
> AArch64 (in a following patch).
> Bootstrap and test on AArch64.  Is it OK?  BTW, this patch is necessary for 
> gcc.dg/vect/PR56541.c (on AArch64) which was added before in tree 
> if-conversion patch.
>
> Thanks,
> bin
>
> 2016-05-11  Alan Lawrence  
> Renlin Li  
> Bin Cheng  
>
> * config/aarch64/iterators.md (V_cmp_mixed, v_cmp_mixed): New.
> * config/aarch64/aarch64-simd.md (v2di3): Call
> gen_vcondv2div2di instead of gen_aarch64_vcond_internalv2div2di.
> (aarch64_vcond_internal): Delete pattern.
> (aarch64_vcond_internal): Ditto.
> (vcond_mask_): New pattern.
> (vec_cmp_internal, vec_cmp): New pattern.
> (vec_cmp_internal): New pattern.
> (vec_cmp, vec_cmpu): New pattern.
> (vcond): Re-implement using vec_cmp and vcond_mask.
> (vcondu): Ditto.
> (vcond): Delete.
> (vcond): New pattern.
> (vcondu): New pattern.
> (aarch64_cmtst): Revise comment using aarch64_vcond instead
> of aarch64_vcond_internal.
>
> gcc/testsuite/ChangeLog
> 2016-05-11  Bin Cheng  
>
> * gcc.target/aarch64/vect-vcond.c: New test.

[PATCH AArch64]Support missing vcond pattern by adding/using vec_cmp/vcond_mask patterns.

[Bin Cheng]

Hi,
Alan and Renlin noticed that some vcond patterns are not supported in 
AArch64(or AArch32?) backend, and they both had some patches fixing this.  
After investigation, I agree with them that vcond/vcondu in AArch64's backend 
should be re-implemented using vec_cmp/vcond_mask patterns, so here comes this 
patch which is based on Alan's.  This patch supports all vcond/vcondu patterns 
by implementing/using vec_cmp and vcond_mask patterns.  Different to the 
original patch, it doesn't change GCC's expanding process, and it keeps vcond 
patterns.  The patch also introduces vec_cmp*_internal to support special case 
optimization for vcond/vcondu which current implementation does.
Apart from Alan's patch, I also learned ideas from Renlin's, and it is my 
change that shall be blamed if any potential bug is introduced.

With this patch, GCC's test condition "vect_cond_mixed" can be enabled on 
AArch64 (in a following patch).
Bootstrap and test on AArch64.  Is it OK?  BTW, this patch is necessary for 
gcc.dg/vect/PR56541.c (on AArch64) which was added before in tree if-conversion 
patch.

Thanks,
bin

2016-05-11  Alan Lawrence  
Renlin Li  
Bin Cheng  

* config/aarch64/iterators.md (V_cmp_mixed, v_cmp_mixed): New.
* config/aarch64/aarch64-simd.md (v2di3): Call
gen_vcondv2div2di instead of gen_aarch64_vcond_internalv2div2di.
(aarch64_vcond_internal): Delete pattern.
(aarch64_vcond_internal): Ditto.
(vcond_mask_): New pattern.
(vec_cmp_internal, vec_cmp): New pattern.
(vec_cmp_internal): New pattern.
(vec_cmp, vec_cmpu): New pattern.
(vcond): Re-implement using vec_cmp and vcond_mask.
(vcondu): Ditto.
(vcond): Delete.
(vcond): New pattern.
(vcondu): New pattern.
(aarch64_cmtst): Revise comment using aarch64_vcond instead
of aarch64_vcond_internal.

gcc/testsuite/ChangeLog
2016-05-11  Bin Cheng  

* gcc.target/aarch64/vect-vcond.c: New test.diff --git a/gcc/config/aarch64/aarch64-simd.md 
b/gcc/config/aarch64/aarch64-simd.md
index bd73bce..f51473a 100644
--- a/gcc/config/aarch64/aarch64-simd.md
+++ b/gcc/config/aarch64/aarch64-simd.md
@@ -1053,7 +1053,7 @@
 }
 
   cmp_fmt = gen_rtx_fmt_ee (cmp_operator, V2DImode, operands[1], operands[2]);
-  emit_insn (gen_aarch64_vcond_internalv2div2di (operands[0], operands[1],
+  emit_insn (gen_vcondv2div2di (operands[0], operands[1],
   operands[2], cmp_fmt, operands[1], operands[2]));
   DONE;
 })
@@ -2225,204 +2225,215 @@
   DONE;
 })
 
-(define_expand "aarch64_vcond_internal"
+(define_expand "vcond_mask_"
+  [(match_operand:VALLDI 0 "register_operand")
+   (match_operand:VALLDI 1 "nonmemory_operand")
+   (match_operand:VALLDI 2 "nonmemory_operand")
+   (match_operand: 3 "register_operand")]
+  "TARGET_SIMD"
+{
+  /* If we have (a = (P) ? -1 : 0);
+ Then we can simply move the generated mask (result must be int).  */
+  if (operands[1] == CONSTM1_RTX (mode)
+  && operands[2] == CONST0_RTX (mode))
+emit_move_insn (operands[0], operands[3]);
+  /* Similarly, (a = (P) ? 0 : -1) is just inverting the generated mask.  */
+  else if (operands[1] == CONST0_RTX (mode)
+  && operands[2] == CONSTM1_RTX (mode))
+emit_insn (gen_one_cmpl2 (operands[0], operands[3]));
+  else
+{
+  if (!REG_P (operands[1]))
+   operands[1] = force_reg (mode, operands[1]);
+  if (!REG_P (operands[2]))
+   operands[2] = force_reg (mode, operands[2]);
+  emit_insn (gen_aarch64_simd_bsl (operands[0], operands[3],
+operands[1], operands[2]));
+}
+
+  DONE;
+})
+
+;; Patterns comparing two vectors to produce a mask.
+
+(define_expand "vec_cmp_internal"
   [(set (match_operand:VSDQ_I_DI 0 "register_operand")
-   (if_then_else:VSDQ_I_DI
- (match_operator 3 "comparison_operator"
-   [(match_operand:VSDQ_I_DI 4 "register_operand")
-(match_operand:VSDQ_I_DI 5 "nonmemory_operand")])
- (match_operand:VSDQ_I_DI 1 "nonmemory_operand")
- (match_operand:VSDQ_I_DI 2 "nonmemory_operand")))]
+ (match_operator 1 "comparison_operator"
+   [(match_operand:VSDQ_I_DI 2 "register_operand")
+(match_operand:VSDQ_I_DI 3 "nonmemory_operand")]))]
   "TARGET_SIMD"
 {
-  rtx op1 = operands[1];
-  rtx op2 = operands[2];
-  rtx mask = gen_reg_rtx (mode);
-  enum rtx_code code = GET_CODE (operands[3]);
+  rtx mask = operands[0];
+  enum rtx_code code = GET_CODE (operands[1]);
 
-  /* Switching OP1 and OP2 is necessary for NE (to output a cmeq insn),
- and desirable for other comparisons if it results in FOO ? -1 : 0
- (this allows direct use of the comparison result without a bsl).  */
-  if (code == NE
-  || (code != EQ
- && op1 == CONST0_RTX (mode)
- && op2 == CONSTM1_RTX (mode)))
+  if