Fix aarch64_simd_reg_or_zero predicate (PR fortran/84565)

Message ID 20180227091251.GD5867@tucnak
State New
Headers show
Series
  • Fix aarch64_simd_reg_or_zero predicate (PR fortran/84565)
Related show

Commit Message

Jakub Jelinek Feb. 27, 2018, 9:12 a.m.
Hi!

The following testcase ICEs, because the aarch64_cmeqdf instruction
starting with r256612 no longer accepts CONST0_RTX (E_DFmode) as
valid argument, but the expander generates it anyway.

The bug has been introduced during the addition of aarch64_simd_imm_zero
and aarch64_simd_or_scalar_imm_zero predicates, before the predicate
used to be:
(define_predicate "aarch64_simd_reg_or_zero"
  (and (match_code "reg,subreg,const_int,const_double,const_vector")
       (ior (match_operand 0 "register_operand")
           (ior (match_test "op == const0_rtx")
                (match_test "aarch64_simd_imm_zero_p (op, mode)")))))
with
bool
aarch64_simd_imm_zero_p (rtx x, machine_mode mode)
{
  return x == CONST0_RTX (mode);
}
and so matched not just const,const_vector zeros, but also
const_int zero (that is through op == const0_rtx) and const_double
zero too.

Bootstrapped/regtested on aarch64-linux (scratch Fedora gcc 8 package build
with the patch applied), ok for trunk?

2018-02-27  Jakub Jelinek  <jakub@redhat.com>

	PR fortran/84565
	* config/aarch64/predicates.md (aarch64_simd_reg_or_zero): Use
	aarch64_simd_or_scalar_imm_zero rather than aarch64_simd_imm_zero.

	* gfortran.dg/pr84565.f90: New test.


	Jakub

Comments

Richard Sandiford Feb. 27, 2018, 2:22 p.m. | #1
Jakub Jelinek <jakub@redhat.com> writes:
> Hi!

>

> The following testcase ICEs, because the aarch64_cmeqdf instruction

> starting with r256612 no longer accepts CONST0_RTX (E_DFmode) as

> valid argument, but the expander generates it anyway.

>

> The bug has been introduced during the addition of aarch64_simd_imm_zero

> and aarch64_simd_or_scalar_imm_zero predicates, before the predicate

> used to be:

> (define_predicate "aarch64_simd_reg_or_zero"

>   (and (match_code "reg,subreg,const_int,const_double,const_vector")

>        (ior (match_operand 0 "register_operand")

>            (ior (match_test "op == const0_rtx")

>                 (match_test "aarch64_simd_imm_zero_p (op, mode)")))))

> with

> bool

> aarch64_simd_imm_zero_p (rtx x, machine_mode mode)

> {

>   return x == CONST0_RTX (mode);

> }

> and so matched not just const,const_vector zeros, but also

> const_int zero (that is through op == const0_rtx) and const_double

> zero too.


Thanks for fixing this.

> Bootstrapped/regtested on aarch64-linux (scratch Fedora gcc 8 package build

> with the patch applied), ok for trunk?

>

> 2018-02-27  Jakub Jelinek  <jakub@redhat.com>

>

> 	PR fortran/84565

> 	* config/aarch64/predicates.md (aarch64_simd_reg_or_zero): Use

> 	aarch64_simd_or_scalar_imm_zero rather than aarch64_simd_imm_zero.

>

> 	* gfortran.dg/pr84565.f90: New test.

>

> --- gcc/config/aarch64/predicates.md.jj	2018-02-06 13:13:06.305751221 +0100

> +++ gcc/config/aarch64/predicates.md	2018-02-26 16:30:01.902195208 +0100

> @@ -395,7 +395,7 @@ (define_predicate "aarch64_simd_reg_or_z

>    (and (match_code "reg,subreg,const_int,const_double,const,const_vector")

>         (ior (match_operand 0 "register_operand")

>  	    (match_test "op == const0_rtx")

> -	    (match_operand 0 "aarch64_simd_imm_zero"))))

> +	    (match_operand 0 "aarch64_simd_or_scalar_imm_zero"))))


I think this makes the match_test on the line above redundant.

LGTM otherwise (but I can't approve).  We should probably clean up
the predicates so that the zero checks are more consistent.  genrecog
could probably help by ignoring predicate codes that obviously don't
apply (CONST_INT for vectors, CONST_VECTOR for ints, etc.).  But that's
obviously all stage 1 stuff.

Richard
Jakub Jelinek Feb. 28, 2018, 7:03 p.m. | #2
On Tue, Feb 27, 2018 at 02:22:22PM +0000, Richard Sandiford wrote:
> > Bootstrapped/regtested on aarch64-linux (scratch Fedora gcc 8 package build

> > with the patch applied), ok for trunk?

> >

> > 2018-02-27  Jakub Jelinek  <jakub@redhat.com>

> >

> > 	PR fortran/84565

> > 	* config/aarch64/predicates.md (aarch64_simd_reg_or_zero): Use

> > 	aarch64_simd_or_scalar_imm_zero rather than aarch64_simd_imm_zero.

> >

> > 	* gfortran.dg/pr84565.f90: New test.

> >

> > --- gcc/config/aarch64/predicates.md.jj	2018-02-06 13:13:06.305751221 +0100

> > +++ gcc/config/aarch64/predicates.md	2018-02-26 16:30:01.902195208 +0100

> > @@ -395,7 +395,7 @@ (define_predicate "aarch64_simd_reg_or_z

> >    (and (match_code "reg,subreg,const_int,const_double,const,const_vector")

> >         (ior (match_operand 0 "register_operand")

> >  	    (match_test "op == const0_rtx")

> > -	    (match_operand 0 "aarch64_simd_imm_zero"))))

> > +	    (match_operand 0 "aarch64_simd_or_scalar_imm_zero"))))

> 

> I think this makes the match_test on the line above redundant.


Yes, it is; it used to be redundant before too.  I can change it if needed,
it would be just a matter of dropping the (match_test "op == const0_rtx")
line.

	Jakub
Jakub Jelinek March 6, 2018, 9:26 a.m. | #3
Hi!

I'd like to ping this patch, without or with the additional redundant
(match_test "op == const0_rtx")
line removal.

> Bootstrapped/regtested on aarch64-linux (scratch Fedora gcc 8 package build

> with the patch applied), ok for trunk?

> 

> 2018-02-27  Jakub Jelinek  <jakub@redhat.com>

> 

> 	PR fortran/84565

> 	* config/aarch64/predicates.md (aarch64_simd_reg_or_zero): Use

> 	aarch64_simd_or_scalar_imm_zero rather than aarch64_simd_imm_zero.

> 

> 	* gfortran.dg/pr84565.f90: New test.

> 

> --- gcc/config/aarch64/predicates.md.jj	2018-02-06 13:13:06.305751221 +0100

> +++ gcc/config/aarch64/predicates.md	2018-02-26 16:30:01.902195208 +0100

> @@ -395,7 +395,7 @@ (define_predicate "aarch64_simd_reg_or_z

>    (and (match_code "reg,subreg,const_int,const_double,const,const_vector")

>         (ior (match_operand 0 "register_operand")

>  	    (match_test "op == const0_rtx")

> -	    (match_operand 0 "aarch64_simd_imm_zero"))))

> +	    (match_operand 0 "aarch64_simd_or_scalar_imm_zero"))))

>  

>  (define_predicate "aarch64_simd_struct_operand"

>    (and (match_code "mem")

> --- gcc/testsuite/gfortran.dg/pr84565.f90.jj	2018-02-26 16:32:49.912271950 +0100

> +++ gcc/testsuite/gfortran.dg/pr84565.f90	2018-02-26 16:31:15.423223943 +0100

> @@ -0,0 +1,7 @@

> +! PR fortran/84565

> +! { dg-do compile { target aarch64*-*-* } }

> +! { dg-options "-mlow-precision-sqrt -funsafe-math-optimizations" }

> +subroutine mysqrt(a)

> + real(KIND=KIND(0.0D0)) :: a

> + a=sqrt(a)

> +end subroutine


	Jakub
James Greenhalgh March 7, 2018, 10:36 a.m. | #4
On Tue, Feb 27, 2018 at 02:22:22PM +0000, Richard Sandiford wrote:
> Jakub Jelinek <jakub@redhat.com> writes:

> > Hi!

> >

> > The following testcase ICEs, because the aarch64_cmeqdf instruction

> > starting with r256612 no longer accepts CONST0_RTX (E_DFmode) as

> > valid argument, but the expander generates it anyway.

> >

> > The bug has been introduced during the addition of aarch64_simd_imm_zero

> > and aarch64_simd_or_scalar_imm_zero predicates, before the predicate

> > used to be:

> > (define_predicate "aarch64_simd_reg_or_zero"

> >   (and (match_code "reg,subreg,const_int,const_double,const_vector")

> >        (ior (match_operand 0 "register_operand")

> >            (ior (match_test "op == const0_rtx")

> >                 (match_test "aarch64_simd_imm_zero_p (op, mode)")))))

> > with

> > bool

> > aarch64_simd_imm_zero_p (rtx x, machine_mode mode)

> > {

> >   return x == CONST0_RTX (mode);

> > }

> > and so matched not just const,const_vector zeros, but also

> > const_int zero (that is through op == const0_rtx) and const_double

> > zero too.

> 

> Thanks for fixing this.

> 

> > Bootstrapped/regtested on aarch64-linux (scratch Fedora gcc 8 package build

> > with the patch applied), ok for trunk?

> >

> > 2018-02-27  Jakub Jelinek  <jakub@redhat.com>

> >

> > 	PR fortran/84565

> > 	* config/aarch64/predicates.md (aarch64_simd_reg_or_zero): Use

> > 	aarch64_simd_or_scalar_imm_zero rather than aarch64_simd_imm_zero.

> >

> > 	* gfortran.dg/pr84565.f90: New test.

> >

> > --- gcc/config/aarch64/predicates.md.jj	2018-02-06 13:13:06.305751221 +0100

> > +++ gcc/config/aarch64/predicates.md	2018-02-26 16:30:01.902195208 +0100

> > @@ -395,7 +395,7 @@ (define_predicate "aarch64_simd_reg_or_z

> >    (and (match_code "reg,subreg,const_int,const_double,const,const_vector")

> >         (ior (match_operand 0 "register_operand")

> >  	    (match_test "op == const0_rtx")

> > -	    (match_operand 0 "aarch64_simd_imm_zero"))))

> > +	    (match_operand 0 "aarch64_simd_or_scalar_imm_zero"))))

> 

> I think this makes the match_test on the line above redundant.

> 

> LGTM otherwise (but I can't approve).  We should probably clean up

> the predicates so that the zero checks are more consistent.  genrecog

> could probably help by ignoring predicate codes that obviously don't

> apply (CONST_INT for vectors, CONST_VECTOR for ints, etc.).  But that's

> obviously all stage 1 stuff.


OK.

Thanks,
James

Patch

--- gcc/config/aarch64/predicates.md.jj	2018-02-06 13:13:06.305751221 +0100
+++ gcc/config/aarch64/predicates.md	2018-02-26 16:30:01.902195208 +0100
@@ -395,7 +395,7 @@  (define_predicate "aarch64_simd_reg_or_z
   (and (match_code "reg,subreg,const_int,const_double,const,const_vector")
        (ior (match_operand 0 "register_operand")
 	    (match_test "op == const0_rtx")
-	    (match_operand 0 "aarch64_simd_imm_zero"))))
+	    (match_operand 0 "aarch64_simd_or_scalar_imm_zero"))))
 
 (define_predicate "aarch64_simd_struct_operand"
   (and (match_code "mem")
--- gcc/testsuite/gfortran.dg/pr84565.f90.jj	2018-02-26 16:32:49.912271950 +0100
+++ gcc/testsuite/gfortran.dg/pr84565.f90	2018-02-26 16:31:15.423223943 +0100
@@ -0,0 +1,7 @@ 
+! PR fortran/84565
+! { dg-do compile { target aarch64*-*-* } }
+! { dg-options "-mlow-precision-sqrt -funsafe-math-optimizations" }
+subroutine mysqrt(a)
+ real(KIND=KIND(0.0D0)) :: a
+ a=sqrt(a)
+end subroutine