[AArch64] Fix for gcc-7 regression PR 80530

Message ID 28052a23-3c02-24f6-e699-1f29b0634520@arm.com
State New
Headers show

Commit Message

Richard Earnshaw (lists) April 27, 2017, 12:32 p.m.
This patch fixes the regression caused by the changes to add square root
estimation when compiling for xgene-1 or exynos-m1 targets.

The issue is that the expand path for the reciprocal estimate square
root pattern assumes that pattern cannot fail once it has been decided
that this expansion path is available, but because the logic deep inside
aarch64_emit_approx_sqrt() differs from use_rsqrt_p() the two disagree
as to what is safe.

This patch refactors the logic to ensure that we cannot unknowingly make
different choices here.

Bootstrap/testing completed ok.  I'll apply this to trunk.

Jakub: Are we having an RC2?  If so, is this ok for the gcc-7 branch?

	PR target/80530
	* config/aarch64/aarch64.c (aarch64_emit_approx_sqrt): Ensure
	that the logic for permitting reciprocal estimates matches that
	in use_rsqrt_p.

R.

Comments

Jakub Jelinek April 27, 2017, 1:26 p.m. | #1
On Thu, Apr 27, 2017 at 01:32:11PM +0100, Richard Earnshaw (lists) wrote:
> This patch fixes the regression caused by the changes to add square root

> estimation when compiling for xgene-1 or exynos-m1 targets.

> 

> The issue is that the expand path for the reciprocal estimate square

> root pattern assumes that pattern cannot fail once it has been decided

> that this expansion path is available, but because the logic deep inside

> aarch64_emit_approx_sqrt() differs from use_rsqrt_p() the two disagree

> as to what is safe.

> 

> This patch refactors the logic to ensure that we cannot unknowingly make

> different choices here.

> 

> Bootstrap/testing completed ok.  I'll apply this to trunk.

> 

> Jakub: Are we having an RC2?  If so, is this ok for the gcc-7 branch?

> 

> 	PR target/80530

> 	* config/aarch64/aarch64.c (aarch64_emit_approx_sqrt): Ensure

> 	that the logic for permitting reciprocal estimates matches that

> 	in use_rsqrt_p.


No testcase?

We will have RC2 (later today or tomorrow), so I guess this is ok if you
believe it is important enough to have in 7.1 and very low risk.

	Jakub
Richard Earnshaw (lists) April 27, 2017, 2:04 p.m. | #2
On 27/04/17 14:26, Jakub Jelinek wrote:
> On Thu, Apr 27, 2017 at 01:32:11PM +0100, Richard Earnshaw (lists) wrote:

>> This patch fixes the regression caused by the changes to add square root

>> estimation when compiling for xgene-1 or exynos-m1 targets.

>>

>> The issue is that the expand path for the reciprocal estimate square

>> root pattern assumes that pattern cannot fail once it has been decided

>> that this expansion path is available, but because the logic deep inside

>> aarch64_emit_approx_sqrt() differs from use_rsqrt_p() the two disagree

>> as to what is safe.

>>

>> This patch refactors the logic to ensure that we cannot unknowingly make

>> different choices here.

>>

>> Bootstrap/testing completed ok.  I'll apply this to trunk.

>>

>> Jakub: Are we having an RC2?  If so, is this ok for the gcc-7 branch?

>>

>> 	PR target/80530

>> 	* config/aarch64/aarch64.c (aarch64_emit_approx_sqrt): Ensure

>> 	that the logic for permitting reciprocal estimates matches that

>> 	in use_rsqrt_p.

> 

> No testcase?

> 

> We will have RC2 (later today or tomorrow), so I guess this is ok if you

> believe it is important enough to have in 7.1 and very low risk.

> 

> 	Jakub

> 


Sorry, I should have said that this fixes an ICE with
gcc.dg/tree-ssa/recip-1.c on affected platforms.

Committing now.

R.

Patch hide | download patch | download mbox

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 1e58e9d..6ef49da 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -7971,33 +7971,40 @@  aarch64_emit_approx_sqrt (rtx dst, rtx src, bool recp)
   machine_mode mode = GET_MODE (dst);
 
   if (GET_MODE_INNER (mode) == HFmode)
-    return false;
+    {
+      gcc_assert (!recp);
+      return false;
+    }
 
-  machine_mode mmsk = mode_for_vector
-		        (int_mode_for_mode (GET_MODE_INNER (mode)),
-			 GET_MODE_NUNITS (mode));
-  bool use_approx_sqrt_p = (!recp
-			    && (flag_mlow_precision_sqrt
-			        || (aarch64_tune_params.approx_modes->sqrt
-				    & AARCH64_APPROX_MODE (mode))));
-  bool use_approx_rsqrt_p = (recp
-			     && (flag_mrecip_low_precision_sqrt
-				 || (aarch64_tune_params.approx_modes->recip_sqrt
-				     & AARCH64_APPROX_MODE (mode))));
+  machine_mode mmsk
+    = mode_for_vector (int_mode_for_mode (GET_MODE_INNER (mode)),
+		       GET_MODE_NUNITS (mode));
+  if (!recp)
+    {
+      if (!(flag_mlow_precision_sqrt
+	    || (aarch64_tune_params.approx_modes->sqrt
+		& AARCH64_APPROX_MODE (mode))))
+	return false;
+
+      if (flag_finite_math_only
+	  || flag_trapping_math
+	  || !flag_unsafe_math_optimizations
+	  || optimize_function_for_size_p (cfun))
+	return false;
+    }
+  else
+    /* Caller assumes we cannot fail.  */
+    gcc_assert (use_rsqrt_p (mode));
 
-  if (!flag_finite_math_only
-      || flag_trapping_math
-      || !flag_unsafe_math_optimizations
-      || !(use_approx_sqrt_p || use_approx_rsqrt_p)
-      || optimize_function_for_size_p (cfun))
-    return false;
 
   rtx xmsk = gen_reg_rtx (mmsk);
   if (!recp)
-    /* When calculating the approximate square root, compare the argument with
-       0.0 and create a mask.  */
-    emit_insn (gen_rtx_SET (xmsk, gen_rtx_NEG (mmsk, gen_rtx_EQ (mmsk, src,
-							  CONST0_RTX (mode)))));
+    /* When calculating the approximate square root, compare the
+       argument with 0.0 and create a mask.  */
+    emit_insn (gen_rtx_SET (xmsk,
+			    gen_rtx_NEG (mmsk,
+					 gen_rtx_EQ (mmsk, src,
+						     CONST0_RTX (mode)))));
 
   /* Estimate the approximate reciprocal square root.  */
   rtx xdst = gen_reg_rtx (mode);