msp430 port

[Richard Henderson]

On 07/23/2013 12:08 PM, DJ Delorie wrote:
> ===================================================================
> --- gcc/simplify-rtx.c	(revision 201184)
> +++ gcc/simplify-rtx.c	(working copy)
> @@ -5884,12 +5884,23 @@ simplify_immed_subreg (enum machine_mode
>  /* Simplify SUBREG:OUTERMODE(OP:INNERMODE, BYTE)
>     Return 0 if no simplifications are possible.  */
>  rtx
>  simplify_subreg (enum machine_mode outermode, rtx op,
>  		 enum machine_mode innermode, unsigned int byte)
>  {
> +  /* FIXME: hack to allow building of newlib/libc.a for msp430/430x/large multilib.
> +     The problem is the var-tracking is generating paradoxical SUBREGs.  Not sure why...  */
> +  if (!(GET_MODE (op) == innermode
> +        || GET_MODE (op) == VOIDmode)
> +      || (innermode == VOIDmode)
> +      )
> +    {
> +      debug_rtx (op);
> +      return NULL_RTX;
> +    }

Jeff asked about this one.
Certainly this sort of hack could never go in as-is.

> +(define_predicate "msp_volatile_memory_operand"
> +  (and (match_code "mem")
> +       (match_test ("memory_address_addr_space_p (GET_MODE (op), XEXP (op, 0), MEM_ADDR_SPACE (op))")))
> +)

The name could be better here.  Alternately,

> +; TRUE for any valid general operand.  We do this because
> +; general_operand refuses to match volatile memory refs.
> +
> +(define_predicate "msp_general_operand"
> +  (ior (match_operand 0 "general_operand")
> +       (match_operand 0 "msp_volatile_memory_operand"))
> +)

... define these as

(define_predicate "msp_general_operand"
  (match_code "mem,reg,subreg,const_int,const,symbol_ref,label_ref"
{
  int save_volatile_ok = volatile_ok;
  volatile_ok = 1;
  int ret = general_operand (op, mode);
  volatile_ok = save_volatile_ok;
  return ret;
})

That said, why do they exist?

> +; TRUE for constants which are bit positions for zero_extract
> +(define_predicate "msp430_bitpos"
> +  (and (match_code "const_int")
> +       (match_test ("   INTVAL (op) >= 0
> +		     && INTVAL (op) <= 15 "))))

IN_RANGE?

> +;; This is nasty.  Operand0 is bogus.  It is only there so that we can get a
> +;; mode for the %B0 to work.  We should use operand1 for this, but that does
> +;; not have a mode.
> +;; 
> +;; Operand1 is actually a register, but we cannot accept (REG...) because the
> +;; cprop_hardreg pass can and will renumber registers even inside
> +;; unspec_volatiles.  So we take an integer register number parameter and
> +;; fudge it to be a register name when we generate the assembler.
> +;;
> +;; The pushm pattern does not have this problem because of all of the
> +;; frame info cruft attached to it, so cprop_hardreg leaves it alone.
> +(define_insn "popm"
> +  [(unspec_volatile [(match_operand 0 "register_operand" "r")
> +		     (match_operand 1 "immediate_operand" "i")
> +		     (match_operand 2 "immediate_operand" "i")] UNS_POPM)]
> +  ""
> +  "POPM%B0\t%2, r%D1"
> +  )

That's because the insn is mis-defined.  Even with unspec_volatile, you must
respect the form of rtl.  Above, operand 1 is positioned as an input, which is
wrong.  It must be positioned as an output, e.g.

  [(set (match_operand 0 "register_operand" "=r")
        (unspec_volatile [(match_operand 1 "immediate_operand" "i")]
                         UNS_POPM))]
  "POPM%B0\t%1, %0

> +;; The next two patterns are here to support a "feature" of how GCC implements
> +;; varargs.  When a function uses varargs and the *second* to last named
> +;; argument is split between argument registers and the stack, gcc expects the
> +;; callee to allocate space on the stack that can contain the register-based
> +;; part of the argument.  This space *has* to be just before the remaining
> +;; arguments (ie the ones that are fully on the stack).

Ug.  We ought to have been able to convince the compiler to copy the other
direction.  I.e. create storage for that argument as if it were a regular local
variable, spill the register portion into it, and also copy in the fragment
from the stack.

That sort of solution seems better than fiddling the location of the return
address in both directions.

> +; This pattern is needed in order to avoid reload problems.
> +; It takes an SI pair of registers, adds a value to them, and
> +; then converts them into a single PSI register.
> +
> +(define_insn "addsipsi3"
> +  [(set (subreg:SI (match_operand:PSI 0 "register_operand" "=&r") 0)
> +	(plus:SI (match_operand:SI    1 "register_operand" "0")
> +		 (match_operand       2 "general_operand" "rmi")))
> +   (clobber (reg:CC CARRY))

Do you really need the subreg here?  Or would you be better off with
a truncate pattern on the RHS?  Alternately, Jeff's solution.

> +(define_split
> +  [(set (match_operand:SI 0 "msp430_nonsubreg_operand" "=&rm")
> +	(plus:SI (match_operand:SI 1 "nonimmediate_operand" "%0")
> +		 (match_operand:SI 2 "general_operand" "rmi")))

No constraints on splits.

> +   (clobber (reg:CC CARRY))
> +   ]
> +  ""
> +  [(parallel [(set (match_operand:HI 3 "nonimmediate_operand" "=&rm")

Nor here.

> +;; Alternatives 2 and 3 are to handle cases generated by reload.
> +(define_insn "subpsi3"
> +  [(set (match_operand:PSI            0 "nonimmediate_operand" "=r,   rm, &?r, ?&r")
> +	(minus:PSI (match_operand:PSI 1 "general_operand"       "0,   0,   !r,  !i")
> +		   (match_operand:PSI 2 "general_operand"       "rLs, rmi, rmi,  r")))
> +   (clobber (reg:CC CARRY))

Under what conditions does reload generate 2 & 3?
We don't handle these cases for i386, so I question why
you'd have to handle them here.

> +(define_insn "*bic<mode>_cg"
> +  [(set (match_operand:QHI 0 "msp_nonimmediate_operand" "=rYs,m")
> +	(and:QHI (match_operand:QHI 1 "msp_general_operand" "0,0")
> +		 (match_operand 2 "msp430_inv_constgen_operator" "n,n")))]
> +  ""
> +  "@
> +   BIC%x0%B0\t#%I2, %0
> +   BIC%X0%B0\t#%I2, %0"
> +)

This really should be an alternative of

> +(define_insn "and<mode>3"
> +  [(set (match_operand:QHI 0 "msp_nonimmediate_operand" "=rYs,rm")
> +	(and:QHI (match_operand:QHI 1 "msp_nonimmediate_operand" "%0,0")
> +		 (match_operand:QHI 2 "msp_general_operand" "riYs,rmi")))
> +   (clobber (reg:CC CARRY))
> +   ]
> +  ""
> +  "@
> +   AND%x0%B0\t%2, %0
> +   AND%X0%B0\t%2, %0"
> +)

... this.

> +(define_insn "msp_return"
> +  [(return)]
> +  ""
> +  { return TARGET_LARGE ? "RETA" : "RET"; }
> +)

I suppose shrink-wrapping results in larger prologues and epilogues
in general?  Otherwise I'd suggest using simple_return here.

> +(define_insn "cbranchhi4_reversed"
> +  [(set (pc) (if_then_else
> +	      (match_operator                    0 "msp430_reversible_cmp_operator"
> +			      [(match_operand:HI 1 "general_operand" "rYsi,rmi")
> +			       (match_operand:HI 2 "general_operand" "rYs,rm")])
> +              (label_ref (match_operand          3 "" ""))
> +	      (pc)))
> +   (clobber (reg:CC CARRY))
> +   ]

Do you find these "reversed" patterns actually used?  In the deep dark past
one needed to provide them, but I thought we'd fixed the middle-end not to
generate them anymore...

> +;; These are memory references that are safe to use with the X suffix,
> +;; because we know/assume they need not index across the 64k boundary.
> +(define_constraint "Ys"
> +  "Memory reference, stack only."
> +  (and (match_code "mem")
> +       (ior
> +	(and (match_code "plus" "0")
> +	     (and (match_code "reg" "00")
> +		  (match_test ("CONST_INT_P (XEXP (XEXP (op, 0), 1))"))
> +		  (match_test ("IN_RANGE (INTVAL (XEXP (XEXP (op, 0), 1)), -1 << 15, (1 << 15)-1)"))))
> +	(match_code "reg" "0")
> +	)))

"stack-only" isn't a good description here, surely, since it supports any base
register with minimal offset.

Also, maybe better to use define_memory_constraint, since that way reload is
told that it can reduce any address to a bare base register in order to satisfy
the constraint?

Or does that tend to get in the way of using Ys in alternative 0, and m in
alternative 1?  If so, that fact deserves a comment because it looks like a
mistake otherwise.

> +  /* Which registers need to be saved in the pro/epilogue.  */
> +  int need_to_save [FIRST_PSEUDO_REGISTER];

HARD_REG_SET, surely.

> +#undef  TARGET_FRAME_POINTER_REQUIRED
> +#define TARGET_FRAME_POINTER_REQUIRED msp430_frame_pointer_required
> +
> +static bool
> +msp430_frame_pointer_required (void)
> +{
> +  return false;
> +}

hook_bool_void_false.

> +bool
> +msp430_promote_prototypes (const_tree fntype ATTRIBUTE_UNUSED)
> +{
> +  return false;
> +}

hook_bool_const_tree_false

> +static enum machine_mode
> +msp430_get_raw_arg_mode (int regno)
> +{
> +  return (regno == ARG_POINTER_REGNUM) ? VOIDmode : Pmode;
> +}

How can the raw mode of AP be VOID?


r~