Patchwork RFA: MN10300: Add redundant comparison elimination pass

login
register
mail settings
Submitter Nick Clifton
Date Nov. 10, 2010, 3:11 p.m.
Message ID <4CDAB63C.2010706@redhat.com>
Download mbox | patch
Permalink /patch/70640/
State New
Headers show

Comments

Nick Clifton - Nov. 10, 2010, 3:11 p.m.
Hi Jeff,

> I'd like to see us DTRT, otherwise I suspect this will get dropped on
> the floor.

OK, I'll do that.

> I'd look to catch add/sub and the logicals; anything beyond those
> probably isn't worth the headache.

Sure - I'll have a go at coding this now.

For the record I am attaching the latest version of the reorg version of 
the patch with fixes for all the other points raised by various reviewers.

Cheers
   Nick
Richard Henderson - Nov. 10, 2010, 8:53 p.m.
On 11/10/2010 07:11 AM, Nick Clifton wrote:
> +	  /* Adding 1 or 4 to an address register results in an
> +	     INC/INC4 instruction that doesn't set the flags.  */
...
> +	      /* Some alternatives in the AND pattern use EXTBU which does
> +		 not set the flags.  Hence a CMP following an AND might be
> +		 needed.  */
> +	    case AND:

I was serious about having something in the md file to specify when the
flags are set appropriately.

I think the best solution is to actually modify the pattern and verify
it.  I think this can be done in a completely generic way, creating a
real post-reload flags-combine pass.  Which would mean that we could
apply it to other ports as well; the RX port in particular since that
has regressed since you removed cc0.

I think the new pass should look like:

  (1) It should operate on basic blocks, not the insn stream as you're
      forced to do with its placement in md-reorg.

  (2) All insn patterns for which any alternative modifies the flags
      should be represented as

	[(set (dest) (operation))
	 (clobber (flags:CC))]

      all the way from initial rtl generation.

  (3) Compare patterns should be of the form

	[(set (flags:CC) (compare:CC (reg) (immediate))]

      Normally immediate will be zero, but it seems easy enough to 
      support any value.  Non-support of a second register means we
      don't have to validate as much live state.

  (4) Assume that the mode of the flags register used after splitting
      of cbranch insns (et al) is as if SELECT_CC_MODE were called.

      Initially one could always use plain CC_MODE, but if there is
      variation in the way that flags are set by various insns, a port
      may want to go the way of i386 and specialize.

      For mn10300, it looks like you have two interesting cases: all
      flags set (add, subtract), and NZ flags set but VC flags not
      useful (multiply, shift and logical ops).

  (5) While scanning forward through the block

	(a) Notice insns of form (2) and remember the insn (cc_set_insn)
	and the destination register (cc_set_reg).

	(b) Notice when cc_set_reg is clobbered and forget cc_set_insn.

	(c) Notice when flags register is clobbered, apart from case (a)
	and forget cc_set_insn.

	(d) Notice insns of form (3), a compare.  If cc_set_reg is not
	    the register used, do not eliminate the compare.

	(e) Modify cc_set_insn into

		[(set (dest) (operation))
		 (set (flags:mode) (compare:mode (operation) (immediate)))]

	    where MODE is the existing mode of the flags register as
	    present in the compare insn.  Use validate_change.
	    If the validation succeeds, delete the compare insn.

This does require the md file to define patterns that match the form
in (4e), but at least the rtl data flow will be Correct, which is not
the case following your existing pass.

The effort of duplicating insn patterns could be aided by a bit of
match_parallel magic.  For instance,

(define_insn "*mn10300_xorsi3"
  [(match_parallel 3 "set_or_clobber_flags_operation"
	[(set (match_operand:SI 0 "register_operand" "=dx")
	      (xor:SI (match_operand:SI 1 "register_operand" "%0")
		      (match_operand:SI 2 "nonmemory_operand" "dxi")))])]
    ""
    "xor %2,%0")

(define_predicate "set_or_clobber_flags_operation"
  (match_code "parallel")
{
  rtx op1, flags;

  if (XVECLEN (op, 0) != 2)
    return false;

  op1 = XVECEXP (op, 0, 1);
  if (GET_CODE (op1) == CLOBBER)
    flags = XEXP (op1, 0);
  else if (GET_CODE (op1) == SET)
    {
      if (!rtx_equal_p (SET_SRC (op1), SET_SRC (XVECEXP (op, 0, 0)))
	return false;
      flags = SET_DEST (op1);
    }
  else
    return false;

  return REG_P (flags) && REGNO (flags) == CC_REG;
})

You won't want to use that for things like PLUS and AND though,
since then you'd want to have separate patterns that eliminate
the alternatives that *don't* set the registers, e.g. INC4.


r~

Patch

Index: gcc/config/mn10300/mn10300.c
===================================================================
--- gcc/config/mn10300/mn10300.c	(revision 166474)
+++ gcc/config/mn10300/mn10300.c	(working copy)
@@ -2403,6 +2403,128 @@ 
   /* Extract the latency value from the timings attribute.  */
   return timings < 100 ? (timings % 10) : (timings % 100);
 }
+
+static void
+scan_for_redundant_compares (void)
+{
+  rtx cur_insn;
+
+  /* Look for this sequence:
+
+       (set (reg X)  (arith_op (...)))
+       (set (reg CC) (compare (reg X) (const_int 0)))
+       (set (pc)     (if_then_else (EQ|NE (...)) (...) (...)))
+
+     And remove the compare as the flags in the
+     EPSW register will already be correctly set.  */
+  for (cur_insn = get_insns (); cur_insn != NULL; cur_insn = NEXT_INSN (cur_insn))
+    {
+      rtx pattern;
+
+      if (! INSN_P (cur_insn))
+	continue;
+
+      pattern = PATTERN (cur_insn);
+
+      if (GET_CODE (pattern) == SET
+	  && GET_CODE (SET_SRC (pattern)) == COMPARE
+	  /* Paranoia checks:  */
+	  && REG_P (SET_DEST (pattern))
+	  && REGNO (SET_DEST (pattern)) == CC_REG
+	  && REG_P (XEXP (SET_SRC (pattern), 0))
+	  /* Normal checks:  */
+	  && CONST_INT_P (XEXP (SET_SRC (pattern), 1))
+	  && INTVAL (XEXP (SET_SRC (pattern), 1)) == 0)
+	{
+	  rtx prev_insn, branch, condition;
+	  unsigned int compare_reg;
+
+	  /* FIXME: We should scan backwards until the first ESPW
+	     setter or clobber insn is found (or the beginning of
+	     the block).  At the moment we just look back one insn.  */
+	  prev_insn = prev_nonnote_insn (cur_insn);
+
+	  if (prev_insn == NULL || ! INSN_P (prev_insn))
+	    continue;
+
+	  /* An UNSPEC might be an LIW insn which will not set the
+	     condition code flags in a way that we currently expect.  */
+	  if (GET_CODE (PATTERN (prev_insn)) == UNSPEC)
+	    continue;
+
+	  if (GET_CODE (PATTERN (prev_insn)) != PARALLEL
+	      || XVECLEN (PATTERN (prev_insn), 0) != 2
+	      || GET_CODE (XVECEXP (PATTERN (prev_insn), 0, 0)) != SET)
+	    continue;
+
+	  compare_reg = REGNO (XEXP (SET_SRC (pattern), 0));
+	  pattern = XVECEXP (PATTERN (prev_insn), 0, 0);
+
+	  if (! REG_P (SET_DEST (pattern))
+	      || REGNO (SET_DEST (pattern)) != compare_reg)
+	    continue;
+
+	  branch = next_nonnote_insn (cur_insn);
+	  if (branch == NULL || ! JUMP_P (branch)
+	      || GET_CODE (PATTERN (branch)) != SET
+	      || GET_CODE (SET_SRC (PATTERN (branch))) != IF_THEN_ELSE)
+	    continue;
+	  condition = XEXP (SET_SRC (PATTERN (branch)), 0);
+
+	  switch (GET_CODE (condition))
+	    {
+	    case EQ:
+	    case NE:
+	      break;
+	    default:
+	      continue;
+	    }
+
+	  /* Adding 1 or 4 to an address register results in an
+	     INC/INC4 instruction that doesn't set the flags.  */
+	  if (GET_CODE (SET_SRC (pattern)) == PLUS
+	      && REG_P (SET_DEST (pattern))
+	      && REGNO (SET_DEST (pattern)) >= FIRST_ADDRESS_REGNUM
+	      && REGNO (SET_DEST (pattern)) <= LAST_ADDRESS_REGNUM
+	      && REG_P (XEXP (SET_SRC (pattern), 0))
+	      && REGNO (XEXP (SET_SRC (pattern), 0)) == REGNO (SET_DEST (pattern))
+	      && CONST_INT_P (XEXP (SET_SRC (pattern), 1))
+	      && (INTVAL (XEXP (SET_SRC (pattern), 1)) == 1
+		  || INTVAL (XEXP (SET_SRC (pattern), 1)) == 4))
+	    continue;
+
+	  switch (GET_CODE (SET_SRC (pattern)))
+	    {
+	    case PLUS:
+	    case MINUS:
+	    case MULT:
+#if 0
+	      /* Some alternatives in the AND pattern use EXTBU which does
+		 not set the flags.  Hence a CMP following an AND might be
+		 needed.  */
+	    case AND:
+#endif
+	    case XOR:
+	    case NOT:
+	    case ASHIFT:
+	    case LSHIFTRT:
+	    case ASHIFTRT:
+	      delete_insn (cur_insn);
+	      break;
+	    default:
+	      break;
+	    }
+	}
+    }
+}
+
+/* Implements TARGET_MACHINE_DEPENDENT_REORG.  */
+
+static void
+mn10300_reorg (void)
+{
+  scan_for_redundant_compares ();
+}
 
 /* Initialize the GCC target structure.  */
 
@@ -2481,4 +2603,7 @@ 
 #undef  TARGET_SCHED_ADJUST_COST
 #define TARGET_SCHED_ADJUST_COST mn10300_adjust_sched_cost
 
+#undef  TARGET_MACHINE_DEPENDENT_REORG
+#define TARGET_MACHINE_DEPENDENT_REORG mn10300_reorg
+
 struct gcc_target targetm = TARGET_INITIALIZER;