diff mbox

[AArch64,costs,14/18] Cost comparisons, flag setting operators and IF_THEN_ELSE

Message ID 1395941622-22926-15-git-send-email-james.greenhalgh@arm.com
State New
Headers show

Commit Message

James Greenhalgh March 27, 2014, 5:33 p.m. UTC
Hi,

Next, comparisons, flag setting operations and IF_THEN_ELSE.

Tested on aarch64-none-elf.

Ok for stage 1?

Thanks,
James

---
2014-03-27  James Greenhalgh  <james.greenhalgh@arm.com>
	    Philipp Tomsich  <philipp.tomsich@theobroma-systems.com>

	* config/aarch64/aarch64.c (aarch64_rtx_costs): Cost comparison
	operators.

Comments

Andrew Pinski May 28, 2014, 9:51 p.m. UTC | #1
On Thu, Mar 27, 2014 at 10:33 AM, James Greenhalgh
<james.greenhalgh@arm.com> wrote:
> Hi,
>
> Next, comparisons, flag setting operations and IF_THEN_ELSE.
>
> Tested on aarch64-none-elf.
>
> Ok for stage 1?

This broke building the Linux kernel.
A simple testcase:
unsigned grab_cache_page_write_begin(unsigned flags, unsigned capabilities)
{
  unsigned gfp_mask;
  unsigned gfp_notmask = 0;
  gfp_mask = flags & ((1 << 25) - 1);
  if (!(capabilities & 0x00000001))
    gfp_mask |= 0x1000000u;
  return (gfp_mask & ~gfp_notmask);
}

---- CUT ----
The problem is combine creates the following RTL:
(if_then_else:SI (reg:SI 78 [ D.2578 ])
    (const_int 0 [0])
    (const_int 16777216 [0x1000000]))

Which the code you added does not handle.  I am going to fix this but
I need to re-factor this code.  I am going to place the code for
IF_THEN_ELSE in its own function also since it is getting too large
for my taste.

Thanks,
Andrew Pinski

>
> Thanks,
> James
>
> ---
> 2014-03-27  James Greenhalgh  <james.greenhalgh@arm.com>
>             Philipp Tomsich  <philipp.tomsich@theobroma-systems.com>
>
>         * config/aarch64/aarch64.c (aarch64_rtx_costs): Cost comparison
>         operators.
James Greenhalgh May 28, 2014, 10:19 p.m. UTC | #2
On Wed, May 28, 2014 at 10:51:10PM +0100, Andrew Pinski wrote:
> On Thu, Mar 27, 2014 at 10:33 AM, James Greenhalgh
> <james.greenhalgh@arm.com> wrote:
> > Hi,
> >
> > Next, comparisons, flag setting operations and IF_THEN_ELSE.
> >
> > Tested on aarch64-none-elf.
> >
> > Ok for stage 1?
> 
> This broke building the Linux kernel.
> A simple testcase:
> unsigned grab_cache_page_write_begin(unsigned flags, unsigned capabilities)
> {
>   unsigned gfp_mask;
>   unsigned gfp_notmask = 0;
>   gfp_mask = flags & ((1 << 25) - 1);
>   if (!(capabilities & 0x00000001))
>     gfp_mask |= 0x1000000u;
>   return (gfp_mask & ~gfp_notmask);
> }
> 
> ---- CUT ----
> The problem is combine creates the following RTL:
> (if_then_else:SI (reg:SI 78 [ D.2578 ])
>     (const_int 0 [0])
>     (const_int 16777216 [0x1000000]))
> 
> Which the code you added does not handle.  I am going to fix this but
> I need to re-factor this code.  I am going to place the code for
> IF_THEN_ELSE in its own function also since it is getting too large
> for my taste.

Ugh, sorry for that and confirmed locally. Refactoring this is very sensible.

Thanks for working on the fix.
James

> 
> Thanks,
> Andrew Pinski
> 
> >
> > Thanks,
> > James
> >
> > ---
> > 2014-03-27  James Greenhalgh  <james.greenhalgh@arm.com>
> >             Philipp Tomsich  <philipp.tomsich@theobroma-systems.com>
> >
> >         * config/aarch64/aarch64.c (aarch64_rtx_costs): Cost comparison
> >         operators.
>
diff mbox

Patch

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index c6f1ac5..bdfcc55 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -4833,7 +4833,7 @@  static bool
 aarch64_rtx_costs (rtx x, int code, int outer ATTRIBUTE_UNUSED,
 		   int param ATTRIBUTE_UNUSED, int *cost, bool speed)
 {
-  rtx op0, op1;
+  rtx op0, op1, op2;
   const struct cpu_cost_table *extra_cost
     = aarch64_tune_params->insn_extra_cost;
   enum machine_mode mode = GET_MODE (x);
@@ -5058,16 +5058,77 @@  aarch64_rtx_costs (rtx x, int code, int outer ATTRIBUTE_UNUSED,
 	  goto cost_logic;
 	}
 
-      /* Comparisons can work if the order is swapped.
-	 Canonicalization puts the more complex operation first, but
-	 we want it in op1.  */
-      if (! (REG_P (op0)
-	     || (GET_CODE (op0) == SUBREG && REG_P (SUBREG_REG (op0)))))
-	{
-	  op0 = XEXP (x, 1);
-	  op1 = XEXP (x, 0);
-	}
-      goto cost_minus;
+      if (GET_MODE_CLASS (GET_MODE (op0)) == MODE_INT)
+        {
+          /* TODO: A write to the CC flags possibly costs extra, this
+	     needs encoding in the cost tables.  */
+
+          /* CC_ZESWPmode supports zero extend for free.  */
+          if (GET_MODE (x) == CC_ZESWPmode && GET_CODE (op0) == ZERO_EXTEND)
+            op0 = XEXP (op0, 0);
+
+          /* ANDS.  */
+          if (GET_CODE (op0) == AND)
+            {
+              x = op0;
+              goto cost_logic;
+            }
+
+          if (GET_CODE (op0) == PLUS)
+            {
+	      /* ADDS (and CMN alias).  */
+              x = op0;
+              goto cost_plus;
+            }
+
+          if (GET_CODE (op0) == MINUS)
+            {
+	      /* SUBS.  */
+              x = op0;
+              goto cost_minus;
+            }
+
+          if (GET_CODE (op1) == NEG)
+            {
+	      /* CMN.  */
+	      if (speed)
+		*cost += extra_cost->alu.arith;
+
+              *cost += rtx_cost (op0, COMPARE, 0, speed);
+	      *cost += rtx_cost (XEXP (op1, 0), NEG, 1, speed);
+              return true;
+            }
+
+          /* CMP.
+
+	     Compare can freely swap the order of operands, and
+             canonicalization puts the more complex operation first.
+             But the integer MINUS logic expects the shift/extend
+             operation in op1.  */
+          if (! (REG_P (op0)
+                 || (GET_CODE (op0) == SUBREG && REG_P (SUBREG_REG (op0)))))
+          {
+            op0 = XEXP (x, 1);
+            op1 = XEXP (x, 0);
+          }
+          goto cost_minus;
+        }
+
+      if (GET_MODE_CLASS (GET_MODE (op0)) == MODE_FLOAT)
+        {
+	  /* FCMP.  */
+	  if (speed)
+	    *cost += extra_cost->fp[mode == DFmode].compare;
+
+          if (CONST_DOUBLE_P (op1) && aarch64_float_const_zero_rtx_p (op1))
+            {
+              /* FCMP supports constant 0.0 for no extra cost. */
+              return true;
+            }
+          return false;
+        }
+
+      return false;
 
     case MINUS:
       {
@@ -5138,6 +5199,7 @@  cost_minus:
 	op0 = XEXP (x, 0);
 	op1 = XEXP (x, 1);
 
+cost_plus:
 	if (GET_RTX_CLASS (GET_CODE (op0)) == RTX_COMPARE
 	    || GET_RTX_CLASS (GET_CODE (op0)) == RTX_COMM_COMPARE)
 	  {
@@ -5451,6 +5513,81 @@  cost_minus:
 	}
       return false;  /* All arguments need to be in registers.  */
 
+    case IF_THEN_ELSE:
+      op2 = XEXP (x, 2);
+      op0 = XEXP (x, 0);
+      op1 = XEXP (x, 1);
+
+      if (GET_CODE (op1) == PC || GET_CODE (op2) == PC)
+        {
+          /* Conditional branch.  */
+          if (GET_MODE_CLASS (GET_MODE (XEXP (op0, 0))) == MODE_CC)
+	    return true;
+	  else
+	    {
+	      if (GET_CODE (op0) == NE
+		  || GET_CODE (op0) == EQ)
+		{
+		  rtx inner = XEXP (op0, 0);
+		  rtx comparator = XEXP (op0, 1);
+
+		  if (comparator == const0_rtx)
+		    {
+		      /* TBZ/TBNZ/CBZ/CBNZ.  */
+		      if (GET_CODE (inner) == ZERO_EXTRACT)
+			/* TBZ/TBNZ.  */
+			*cost += rtx_cost (XEXP (inner, 0), ZERO_EXTRACT,
+					   0, speed);
+		      else
+			/* CBZ/CBNZ.  */
+			*cost += rtx_cost (inner, GET_CODE (op0), 0, speed);
+
+		      return true;
+		    }
+		}
+	      else if (GET_CODE (op0) == LT
+		       || GET_CODE (op0) == GE)
+		{
+		  rtx comparator = XEXP (op0, 1);
+
+		  /* TBZ/TBNZ.  */
+		  if (comparator == const0_rtx)
+		    return true;
+		}
+	    }
+        }
+      else if (GET_MODE_CLASS (GET_MODE (XEXP (op0, 0))) == MODE_CC)
+        {
+          /* It's a conditional operation based on the status flags,
+             so it must be some flavor of CSEL.  */
+
+          /* CSNEG, CSINV, and CSINC are handled for free as part of CSEL.  */
+          if (GET_CODE (op1) == NEG
+              || GET_CODE (op1) == NOT
+              || (GET_CODE (op1) == PLUS && XEXP (op1, 1) == const1_rtx))
+            op1 = XEXP (op1, 0);
+
+          *cost += rtx_cost (op1, IF_THEN_ELSE, 1, speed);
+          *cost += rtx_cost (op2, IF_THEN_ELSE, 2, speed);
+          return true;
+        }
+
+      /* We don't know what this is, cost all operands.  */
+      return false;
+
+    case EQ:
+    case NE:
+    case GT:
+    case GTU:
+    case LT:
+    case LTU:
+    case GE:
+    case GEU:
+    case LE:
+    case LEU:
+
+      return false; /* All arguments must be in registers.  */
+
     default:
       break;
     }