[PATCH] Fix aarch64 bootstrap (pr69416)

[PATCH] Fix aarch64 bootstrap (pr69416)

[Richard Henderson]

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

The bare CONST_INT inside the CCmode IF_THEN_ELSE is causing combine to make 
incorrect simplifications.  At this stage it feels safer to wrap the CONST_INT 
inside of an UNSPEC than make more generic changes to combine.

But we should definitely investigate combine's CCmode issues for gcc7.


Ok?


r~

[-- Attachment #2: d-69416 --]
[-- Type: text/plain, Size: 1418 bytes --]

	* config/aarch64/aarch64.md (UNSPEC_NZCV): New.
	(ccmp<mode>, fccmp<mode>, fccmpe<mode>): Use it.
	
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 2f543aa..71fc514 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -129,6 +129,7 @@
     UNSPEC_RSQRT
     UNSPEC_RSQRTE
     UNSPEC_RSQRTS
+    UNSPEC_NZCV
 ])
 
 (define_c_enum "unspecv" [
@@ -280,7 +281,7 @@
 	  (compare:CC
 	    (match_operand:GPI 2 "register_operand" "r,r,r")
 	    (match_operand:GPI 3 "aarch64_ccmp_operand" "r,Uss,Usn"))
-	  (match_operand 5 "immediate_operand")))]
+	  (unspec:CC [(match_operand 5 "immediate_operand")] UNSPEC_NZCV)))]
   ""
   "@
    ccmp\\t%<w>2, %<w>3, %k5, %m4
@@ -298,7 +299,7 @@
 	  (compare:CCFP
 	    (match_operand:GPF 2 "register_operand" "w")
 	    (match_operand:GPF 3 "register_operand" "w"))
-	  (match_operand 5 "immediate_operand")))]
+	  (unspec:CCFP [(match_operand 5 "immediate_operand")] UNSPEC_NZCV)))]
   "TARGET_FLOAT"
   "fccmp\\t%<s>2, %<s>3, %k5, %m4"
   [(set_attr "type" "fcmp<s>")]
@@ -313,7 +314,7 @@
 	   (compare:CCFPE
 	    (match_operand:GPF 2 "register_operand" "w")
 	    (match_operand:GPF 3 "register_operand" "w"))
-	  (match_operand 5 "immediate_operand")))]
+	  (unspec:CCFPE [(match_operand 5 "immediate_operand")] UNSPEC_NZCV)))]
   "TARGET_FLOAT"
   "fccmpe\\t%<s>2, %<s>3, %k5, %m4"
   [(set_attr "type" "fcmp<s>")]

Re: [PATCH] Fix aarch64 bootstrap (pr69416)

[Richard Earnshaw (lists)]

On 22/01/16 17:07, Richard Henderson wrote:
> The bare CONST_INT inside the CCmode IF_THEN_ELSE is causing combine to
> make incorrect simplifications.  At this stage it feels safer to wrap
> the CONST_INT inside of an UNSPEC than make more generic changes to
> combine.
> 
> But we should definitely investigate combine's CCmode issues for gcc7.
> 

Agreed.

> 
> Ok?
> 
OK.

R.

> 
> r~
> 
> d-69416
> 
> 
> 	* config/aarch64/aarch64.md (UNSPEC_NZCV): New.
> 	(ccmp<mode>, fccmp<mode>, fccmpe<mode>): Use it.
> 	
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index 2f543aa..71fc514 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -129,6 +129,7 @@
>      UNSPEC_RSQRT
>      UNSPEC_RSQRTE
>      UNSPEC_RSQRTS
> +    UNSPEC_NZCV
>  ])
>  
>  (define_c_enum "unspecv" [
> @@ -280,7 +281,7 @@
>  	  (compare:CC
>  	    (match_operand:GPI 2 "register_operand" "r,r,r")
>  	    (match_operand:GPI 3 "aarch64_ccmp_operand" "r,Uss,Usn"))
> -	  (match_operand 5 "immediate_operand")))]
> +	  (unspec:CC [(match_operand 5 "immediate_operand")] UNSPEC_NZCV)))]
>    ""
>    "@
>     ccmp\\t%<w>2, %<w>3, %k5, %m4
> @@ -298,7 +299,7 @@
>  	  (compare:CCFP
>  	    (match_operand:GPF 2 "register_operand" "w")
>  	    (match_operand:GPF 3 "register_operand" "w"))
> -	  (match_operand 5 "immediate_operand")))]
> +	  (unspec:CCFP [(match_operand 5 "immediate_operand")] UNSPEC_NZCV)))]
>    "TARGET_FLOAT"
>    "fccmp\\t%<s>2, %<s>3, %k5, %m4"
>    [(set_attr "type" "fcmp<s>")]
> @@ -313,7 +314,7 @@
>  	   (compare:CCFPE
>  	    (match_operand:GPF 2 "register_operand" "w")
>  	    (match_operand:GPF 3 "register_operand" "w"))
> -	  (match_operand 5 "immediate_operand")))]
> +	  (unspec:CCFPE [(match_operand 5 "immediate_operand")] UNSPEC_NZCV)))]
>    "TARGET_FLOAT"
>    "fccmpe\\t%<s>2, %<s>3, %k5, %m4"
>    [(set_attr "type" "fcmp<s>")]
> 

Re: [PATCH] Fix aarch64 bootstrap (pr69416)

[Christophe Lyon]

On 22 January 2016 at 18:07, Richard Henderson <rth@redhat.com> wrote:
> The bare CONST_INT inside the CCmode IF_THEN_ELSE is causing combine to make
> incorrect simplifications.  At this stage it feels safer to wrap the
> CONST_INT inside of an UNSPEC than make more generic changes to combine.
>
> But we should definitely investigate combine's CCmode issues for gcc7.
>
>
> Ok?
>
Hi,

After this, I'm seeing this test now FAILs:
gcc.target/aarch64/ccmp_1.c scan-assembler adds\t

Christophe


>
> r~

Re: [PATCH] Fix aarch64 bootstrap (pr69416)

[Richard Henderson]

On 01/25/2016 05:28 AM, Christophe Lyon wrote:
> After this, I'm seeing this test now FAILs:
> gcc.target/aarch64/ccmp_1.c scan-assembler adds\t

That test case is badly written.  In addition to that one, several of the other
failures that I see within that file are simply equally optimal alternative
choices for the compiler.  The file needs to be split up and simpler more
directed tests written.


r~

Re: [PATCH] Fix aarch64 bootstrap (pr69416)

[Wilco Dijkstra]

Richard Henderson wrote:
> On 01/25/2016 05:28 AM, Christophe Lyon wrote:
> > After this, I'm seeing this test now FAILs:
> > gcc.target/aarch64/ccmp_1.c scan-assembler adds\t
>
> That test case is badly written.  In addition to that one, several of the other
> failures that I see within that file are simply equally optimal alternative
> choices for the compiler.  The file needs to be split up and simpler more
> directed tests written.

The test case was written specifically to emit 'adds' as that is the optimal 
sequence. It is a regression caused by wrapping the immediate in a unspec which
disables costing of all CCMPs... I have a patch for this. 

The zero issue is due to the testcase assuming GCC emits '0' and 'wzr' correctly -
it was based on a very old patch that emits the correct zero for compares that hasn't
been OK'd yet. And the failure to emit an fccmp is due to a recent fix to NaN handling
in compares, so that testcase now needs -ffinite-math-only.

Wilco