RFA: MN10300: Add redundant comparison elimination pass

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

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~