Re: CR16 Port addition

[Richard Henderson <rth@redhat.com>]

> +/* Initialize 'targetm' variable which contains pointers to functions 
> +   and data relating to the target machine.  */
> +struct gcc_target targetm = TARGET_INITIALIZER;

Not a requirement, but as a comment:

I find it much cleaner to move the target initializer to the end
of the file, which avoids the need for forward delcarations of 
the functions that are placed within.

I know there is a lot of variation in this among ports, but this
is something that ought to get cleaned up in the future.

> +cr16_class_likely_spilled_p (reg_class_t rclass)
> +{
> +  if ((rclass) == SHORT_REGS || (rclass) == DOUBLE_BASE_REGS \

Trailing \.  Left over from macro-to-function conversion?

> +  /* To avoid gen_rtx_SUBREG ICE. SI gets splitted to two HIs where 
> +     validate_subreg is invalid with the second HI having offset 2
> +     .Offset applicable to big endian machines.  */
> +  flag_split_wide_types = 0;

I would really like to know more about this.

> +  /* If EH, disable optimization GCSE.  */
> +  if (flag_exceptions)
> +    flag_gcse = 0;

Why?  It works for other targets.

> +/* Compute values for the array save_regs and the variable sum_regs.
> +   The index of save_regs is numbers of register, each will get 1 if we 
> +   need to save it in the current function, 0 if not.  sum_regs is the 
> +   total sum of the registers being saved.  */
> +static void
> +cr16_compute_save_regs (void)
...
> +/* Compute the size of the local area and the size to be adjusted by the
> +   prologue and epilogue.  */
> +static void
> +cr16_compute_frame (void)

I would be very much happier if these functions did not store their results
into global variables.  Consider using a structure, like struct ix86_frame
is used in the i386 port.

> +  if ((mode == Pmode) && (regno == 11))
> +    return 0;

Is there a symbolic name for 11?

> +cr16_function_arg (CUMULATIVE_ARGS * cum, enum machine_mode mode,
> +		   const_tree type, bool named ATTRIBUTE_UNUSED)
> +{
> +  last_parm_in_reg = 0;

Using a global here is definitely wrong.  If you need to store extra
data, then store it in the CUMULATIVE_ARGS structure.  Though in this
case I think all you need to do is share some code between function_arg
and function_arg_advance.

> +      if ((next_param == (tree) 0) && (TREE_VALUE (param) != void_type_node))

s/(tree) 0/NULL_TREE/g

> +/* Constraint for bit instructions.  */
> +#define OK_FOR_Z(OP) \
> +  ((GET_CODE (OP) == MEM && GET_CODE (XEXP (OP, 0)) == CONST_INT) \
> +   || (GET_CODE (OP) == MEM && GET_CODE (XEXP (OP, 0)) == REG) \
> +   || (GET_CODE (OP) == MEM && GET_CODE (XEXP (OP, 0)) == PLUS \
> +       && GET_CODE (XEXP ((XEXP (OP, 0)), 0)) == REG \
> +       && GET_CODE (XEXP ((XEXP (OP, 0)), 1)) == CONST_INT))
...
> +(define_predicate "bit_operand"
> +  (match_code "mem")
> +{
> +  return ((GET_CODE (op) == MEM && OK_FOR_Z (op)));
> +})

These should be merged, since OK_FOR_Z isn't used elsewhere.

I don't see a constraints.md file?  Or any of the legacy macros for
that matter.  Did the file get forgotten in the patch?

> +  [(set (match_operand:SHORT 0 "bit_operand" "=mZ")

The constraint here is almost certainly wrong.  It should be only "=Z".
If you're having problems with this, it means you havn't properly
defined the Z constraint with define_memory_constraint.

> +;; Register operand to avoid subreg with offset byte offset 2 
> +(define_predicate "reg_operand"
> +  (match_operand 0 "register_operand")
> +{
> +  if (GET_CODE(op) == SUBREG
> +      && (REGNO(SUBREG_REG(op)) > 11
> +          && REGNO(SUBREG_REG(op)) < FIRST_PSEUDO_REGISTER)
> +      && SUBREG_BYTE(op) != 0)
> +    return 0;    
> +    return 1;
> +})

This is wrong.  This issue should be handled with CANNOT_CHANGE_MODE_CLASS,
and not with random hacks across the rest of the port.

> +(define_expand "adddi3"
> +  [(set (match_operand:DI 0 "reg_operand" "")
> +	(plus:DI (match_operand:DI 1 "reg_operand" "")
> +		 (match_operand:DI 2 "reg_operand" "")))]
> +  ""
> +  {
> +    if ((GET_CODE (operands[2]) != REG) )
> +      {
> +	operands[2] = force_reg (DImode, operands[2]);
> +      }
> +  }

You should trust the predicates to do the right thing.  You
have constrained operands[2] to be a reg_operand.  Why are
you then checking that it's not a REG?

Indeed, once that's fixed there's no reason not to simply
merge your adddi3 expander with the adddi3_insn pattern.

> +	(plus:SI (mult:SI (sz_xtnd:SI (match_operand:HI 1 "reg_operand" "%r"))
> +			  (sz_xtnd:SI (match_operand:HI 2 "reg_operand" "r")))

% should be used only when the constraints of the two operands
are different.  Otherwise you're just wasting time in reload
making it examine both possibilities.

> +  [(set (match_operand:HI 0 "reg_operand" "=r,=r")
> +	(mult:HI (match_operand:HI 1 "register_and_valid_subreg_byte_operand" "%0,0")
> +		 (match_operand:HI 2 "nonmemory_operand" "r,i")))]

Extra = in operand 0.

> +(define_expand "andhi3"
> +  [(set (match_operand:HI 0 "reg_operand" "")
> +	(and:HI (match_operand:HI 1 "reg_operand" "")
> +		(match_operand:HI 2 "nonmemory_operand" "")))]
> +  ""
> +  "
> +{
> +  if ((GET_CODE (operands[1]) == SUBREG 
> +       && REGNO (operands[1]) >= FIRST_PSEUDO_REGISTER)
> +   && (GET_CODE (operands[2]) == SUBREG
> +       && REGNO (operands[2]) >= FIRST_PSEUDO_REGISTER))
> +    FAIL;
> +}")
> +

What is this for?  Disallowing subregs of pseudos?!?

> +;;T -  prints upper half
> +;;U -  prints lower half
> +(define_insn "anddi3"
> +  [(set (match_operand:DI 0 "reg_operand" "=r")
> +	(and:DI (match_operand:DI 1 "reg_operand" "%0")
> +		(match_operand:DI 2 "nonmemory_operand" "r")))]
> +  ""
> +  "andd\t%T2,%T0\;andd\t%U2,%U0"
> +)

You'd be better off without this, and let the compiler generate
it itself from two SImode ands.  Likewise with the rest of the
logical arithmetic.

> +; (DI, DF) move
> +(define_insn "*mov<mode>_double"
> +  [(set (match_operand:DOUBLE 0 "nonimmediate_operand" "=r, r, r, m")
> +	(match_operand:DOUBLE 1 "gen_operand" "r, <iFD>, m, r"))]
> +  "register_operand (operands[0], DImode) 
> +   || register_operand (operands[0], DFmode)
> +   || register_operand (operands[1], DImode)
> +   || register_operand (operands[1], DFmode)"

This whole pattern has got to get cleaned up.  The best way to do so
is to delete it, and let the generic bits of the compiler handle this
itself via subreg decomposition.  Of course, this requires that you
fix the subreg problems that you're having rather than papering over.

> +(define_insn "cbranch<mode>4"
> +  [(set (pc)
> +	(if_then_else (match_operator 0 "ordered_comparison_operator"
> +		      [(match_operand:CR16IM 1 "register_operand" "r,r")
> +		       (match_operand:CR16IM 2 "nonmemory_operand" "r,n")])
> +		       (label_ref (match_operand 3 "" ""))
> +                      (pc)))
> +   (clobber (cc0))]
> +  ""
> +  "cmp<tIsa>\t%2, %1\;b%d0\t%l3"
> +  [(set_attr "length" "6,6")]
> +)
> +
> +(define_expand "cmp<mode>"
> +  [(parallel [(set (cc0)
> +    (compare (match_operand:CR16IM 0 "register_operand" "")
> +	     (match_operand:CR16IM 1 "register_operand" "")))
> +   (clobber (match_scratch:HI 2 "=r"))]) ]
> +   ""
> +   "")
> +
> +(define_insn "*cmp<mode>_insn"
> +  [(set (cc0)
> +	(compare (match_operand:CR16IM 0 "register_operand" "r,r")
> +		 (match_operand:CR16IM 1 "nonmemory_operand" "r,n")))
> +   (clobber (match_scratch:HI 2 "=r,r")) ]
> +  ""                  
> +  "cmp<tIsa>\t%1, %0"
> +  [(set_attr "length" "2,4")]
> +)

I'm not sure I understand the logic here.  If you have a cbranch insn,
which you never split, I'm not sure why you'd need a cmp pattern at all.

> +(define_insn "indirect_jump_return"
> +  [(parallel
> +    [(set (pc)
> +	  (reg:SI RA_REGNUM))
> +   (return)])
> +  ]
...
> +(define_insn "interrupt_return"
> +  [(parallel
> +    [(unspec_volatile [(const_int 0)] 0)
> +   (return)])]
...
> +(define_insn "push_for_prologue"
> +  [(parallel
> +    [(set (reg:SI SP_REGNUM)
> +	  (minus:SI (reg:SI SP_REGNUM)
> +		    (match_operand:SI 0 "immediate_operand" "i")))])]
...
> +(define_insn "pop_and_popret_return"
> +  [(parallel
> +    [(set (reg:SI SP_REGNUM)
> +	  (plus:SI (reg:SI SP_REGNUM)
> +		   (match_operand:SI 0 "immediate_operand" "i")))
> +     (use (reg:SI RA_REGNUM))
> +     (return)])

Extra parallel.  It's implicit in define_insn.

> +(define_expand "call"
> +  [(call (match_operand:QI 0 "memory_operand" "")
> +	 (match_operand 1 "" ""))]
> +  ""
...
> +(define_expand "cr16_call"
> +  [(parallel
> +    [(call (match_operand:QI 0 "memory_operand" "")
> +	   (match_operand 1 "" ""))
> +   (clobber (reg:SI RA_REGNUM))])]

These can be trivially merged.

> +;;  Nop
> +(define_insn "nop"
> +  [(const_int 0)]
> +  ""
> +  ""
> +)

Nop really needs to output something.  It's used to make sure
various line numbers are distinct addresses at -O0 -g.

> +;; Shared FLAT
> +(define_insn "unspec_library_offset"
> +  [(const_int UNSPEC_LIBRARY_OFFSET)]
> +  ""
> +  "push \t$2, r0;\n\tmovd \t$__current_shared_library_r12_offset_, (r1,r0);\n\tloadd \t[r12]0(r1,r0), (r12);\n\tpop \t$2, r0\n\t"
> +  [(set_attr "length" "12")]
> +)
> +
> +(define_insn "unspec_sh_lib_push_r12"
> +  [(const_int UNSPEC_SH_LIB_PUSH_R12)]
> +  ""
> +  "push \t$2, r12"
> +  [(set_attr "length" "2")]
> +)
> +
> +(define_insn "unspec_sh_lib_pop_r12"
> +  [(const_int UNSPEC_SH_LIB_POP_R12)]
> +  ""
> +  "pop \t$2, r12"
> +  [(set_attr "length" "2")]
> +)

Huh?


r~