Re: PATCH: new port: Moxie

[Ian Lance Taylor <iant at google dot com>]

green@moxielogic.com writes:

> Thanks Paolo.  New version attached below.
>
> Ok to commit?

I've forgotten what the rules are for a new port.  Does it merely
require the approval of a middle-end maintainer, or is something further
required?  In particular, does the SC need to approve the new
maintainership?


At some point you should update http://gcc.gnu.org/backends.html.


+(define_constraint "A"
+  "An absolute address."
+  (and (match_code "mem")
+       (match_test "GET_CODE (XEXP (op, 0)) == SYMBOL_REF
+                  ||GET_CODE (XEXP (op, 0)) == LABEL_REF
+                  ||GET_CODE (XEXP (op, 0)) == CONST")))

Write this as

  (and (match_code "mem")
       (or (match_test "GET_CODE (XEXP (op, 0)) == SYMBOL_REF")
           (match_test "GET_CODE (XEXP (op, 0)) == LABEL_REF")
           (match_test "GET_CODE (XEXP (op, 0)) == CONST")))

+(define_constraint "N"
+  "An constant -(0..255)"
+  (and (match_code "const_int")
+       (match_test "((unsigned int) ival) >= 0xffffff01 && ((unsigned int) ival) <= 0xffffffff")))

Break the long line.


+int compute_frame_size (int, long * ATTRIBUTE_UNUSED);

It seems like this should be static.


+/* Define how to find the value returned by a function.
+   VALTYPE is the data type of the value (as a tree).
+   If the precise function being called is known, FUNC is its
+   FUNCTION_DECL; otherwise, FUNC is 0.  
+
+   We always return values in register $r0 for moxie.  */
+
+rtx
+moxie_function_value (tree valtype, 
+		      tree fntype_or_decl ATTRIBUTE_UNUSED,
+		      bool outgoing ATTRIBUTE_UNUSED)
+{
+  return gen_rtx_REG (TYPE_MODE (valtype), 2);
+}

You might want to consider enum constants or #defines for the registers,
rather than remembering that $r0 is register 2.


+static int moxie_callee_saved_reg_size;

This should not be a static variable, it should be in the
machine_function structure.  You should set the global variable
init_machine_status to point to a function which allocates a struct
machine_function.  That struct should have a field for
callee_saved_reg_size.  You should access it via cfun->machine.

Same with local_vars_size and size_for_adjusting_sp, actually.

+  for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
+    if (df_regs_ever_live_p(regno) && (! call_used_regs[regno]))
+      s += 4;

Space before '('.

+#if 0
+  moxie_debug_stack();
+#endif

There is no moxie_debug_stack function, so you might as well delete
these lines.

+      if (df_regs_ever_live_p(regno) && (! call_used_regs[regno]))

Space before '('.

+void
+moxie_expand_epilogue (void)
+{
+  int regno;
+  rtx insn;
+
+  if (moxie_callee_saved_reg_size != 0)
+    {
+      insn = emit_insn (gen_movsi (gen_rtx_REG (Pmode, 14), GEN_INT (-moxie_callee_saved_reg_size)));
+      RTX_FRAME_RELATED_P (insn) = 1;
+      insn = emit_insn (gen_movsi (stack_pointer_rtx, hard_frame_pointer_rtx));
+      insn = emit_insn (gen_addsi3 (stack_pointer_rtx, stack_pointer_rtx, gen_rtx_REG (Pmode, 14)));
+      RTX_FRAME_RELATED_P (insn) = 1;
+
+      for (regno = FIRST_PSEUDO_REGISTER; regno > 0; --regno)
+	if (df_regs_ever_live_p(regno) && (! call_used_regs[regno]))
+	  {
+	    insn = emit_insn (gen_movsi_pop (gen_rtx_REG (Pmode, regno)));
+	    RTX_FRAME_RELATED_P (insn) = 1;
+	  }
+    }
+
+  insn = emit_jump_insn (gen_returner ());
+  RTX_FRAME_RELATED_P (insn) = 1;
+}

You don't need to set RTX_FRAME_RELATED_P for insns in the epilogue.
However, as far as I know it doesn't hurt, and it may be a good idea if
you ever want to support -fasynchronous-unwind-tables.


+/* Implements the macro INITIAL_ELIMINATION_OFFSET, return the OFFSET. */
+
+int
+moxie_initial_elimination_offset (int from, int to)
+{
+  int ret;
+  
+  /* Compute this since we need to use local_vars_size.  */
+  moxie_compute_frame ();
+
+  if ((from) == FRAME_POINTER_REGNUM && (to) == STACK_POINTER_REGNUM)
+    ret = 0x100;
+  else if ((from) == ARG_POINTER_REGNUM && (to) == FRAME_POINTER_REGNUM)
+    ret = 0x600;
+  else if ((from) == ARG_POINTER_REGNUM && (to) == STACK_POINTER_REGNUM)
+    ret = 0x1000;
+  else if ((from) == FRAME_POINTER_REGNUM && (to) == HARD_FRAME_POINTER_REGNUM)
+    ret = -moxie_callee_saved_reg_size;
+  else if ((from) == ARG_POINTER_REGNUM && (to) == HARD_FRAME_POINTER_REGNUM)
+    ret = 0x00;
+  else
+    abort ();
+
+  return ret;
+}

This function doesn't make any sense as written.  Does this really work?


+/* Do what is necessary for `va_start'.  We look at the current function
+   to determine if stdarg or varargs is used and return the address of the
+   first unnamed parameter.  */
+
+static void
+moxie_setup_incoming_varargs (CUMULATIVE_ARGS *cum,
+			    enum machine_mode mode ATTRIBUTE_UNUSED,
+			    tree type ATTRIBUTE_UNUSED,
+			    int *pretend_size, int no_rtl)
+{

The comment doesn't match what the function does.

+rtx
+moxie_function_arg (CUMULATIVE_ARGS cum, enum machine_mode mode,
+		    tree type ATTRIBUTE_UNUSED, int named ATTRIBUTE_UNUSED)

This function needs a comment.

+static bool
+moxie_pass_by_reference (CUMULATIVE_ARGS *cum ATTRIBUTE_UNUSED,
+		       enum machine_mode mode, const_tree type,
+		       bool named ATTRIBUTE_UNUSED)

So does this one.

+static int
+moxie_arg_partial_bytes (CUMULATIVE_ARGS *cum,
+		       enum machine_mode mode,
+		       tree type, bool named)

And this one.

+#define REG_CLASS_CONTENTS \
+{ { 0x00000000 }, /* Empty */			   \
+  { (1<<18)-1 },  /* $fp, $sp, $r0 to $r5, ?fp */  \
+  { (1<<18) },    /* $pc */	                   \
+  { (1<<19) },    /* ?cc */                        \
+  { (1<<20)-1 }   /* All registers */              \
+}

I think REG_CLASS_CONTENTS is easier to understand if you write out the
hex constants for each value.  Using numberic constants here is
particularly cryptic.

+/* A C expression whose value is a register class containing hard
+   register REGNO.  */
+#define REGNO_REG_CLASS(R) ((R < 18) ? GENERAL_REGS : \
+                            (R == 19? CC_REG : SPECIAL_REGS))

Space before '?'.  Names would be easier to understand than numbers.

+#define INITIAL_FRAME_POINTER_OFFSET(DEPTH) (DEPTH) = 0;

Remove the ';'.


+/* Define this macro as a C expression that is nonzero for registers that are
+   used by the epilogue or the return pattern.  The stack and frame
+   pointer registers are already assumed to be used as needed.  */
+#define EPILOGUE_USES(R) (R==7)

Spaces around '='.

+#define REGNO_OK_FOR_BASE_P(NUM)                 \
+  (HARD_REGNO_OK_FOR_BASE_P(NUM) ||		 \
+   HARD_REGNO_OK_FOR_BASE_P(reg_renumber[(NUM)]))

Move "||" to the start of the last line.

+#define GO_IF_MODE_DEPENDENT_ADDRESS(ADDR,LEBEL)

Don't define this if there is nothing for it to do.

+#define TARGET_CPU_CPP_BUILTINS() \
+  { \
+    builtin_assert ("cpu=moxie"); \
+    builtin_assert ("machine=moxie"); \
+    builtin_define ("__moxie__=1"); \
+  }

asserts were never standard and are now deprecated.  Use
builtin_define's instead.

> +#define NOTICE_UPDATE_CC(EXPR, INSN) 0

You shouldn't need to define this.


+;; Nonzero if OP can be source of a simple move operation.
+
+(define_predicate "moxie_general_movsrc_operand"
+  (match_code "mem,const_int,reg,subreg,symbol_ref,label_ref,const")

define_predicate's are conventionally in predicates.md.

+    if (GET_CODE (operands[0]) == MEM)

Use MEM_P here and elsewhere.

+    {
+      operands[1] = force_reg (SImode, operands[1]);
+      if (GET_CODE (XEXP (operands[0], 0)) == MEM)
+        operands[0] = gen_rtx_MEM (SImode, force_reg (SImode, XEXP (operands[0], 0)));

I don't understand why you would ever see a (MEM (MEM)) here.

+(define_insn "*loadsi_offset"
+  [(set (match_operand:SI 0 "register_operand" "=r")
+ 	(mem:SI (plus:SI
+		  (match_operand:SI 1 "register_operand" "r")
+		  (match_operand:SI 2 "immediate_operand" "i"))))]
+  ""
+  "ldo.l  %0, %2(%1)")
+
+(define_insn "*storesi_offset"
+  [(set (mem:SI (plus:SI
+		  (match_operand:SI 1 "register_operand" "r")
+		  (match_operand:SI 2 "immediate_operand" "i")))
+        (match_operand:SI 0 "register_operand" "r"))]
+  ""
+  "sto.l  %2(%1), %0")

Why are these separate insns, rather than alternates of movsi?

+(define_insn "*loadqi_offset"
+  [(set (match_operand:QI 0 "register_operand" "=r")
+ 	(mem:QI (plus:SI
+		  (match_operand:SI 1 "register_operand" "r")
+		  (match_operand:SI 2 "immediate_operand" "i"))))]
+  ""
+  "ldo.b  %0, %2(%1)")
+
+(define_insn "*storeqi_offset"
+  [(set (mem:QI (plus:SI
+		  (match_operand:SI 1 "register_operand" "r")
+		  (match_operand:SI 2 "immediate_operand" "i")))
+        (match_operand:QI 0 "register_operand" "r"))]
+  ""
+  "sto.b  %2(%1), %0")

Same here, but for movqi.

+(define_insn "*loadhi_offset"
+  [(set (match_operand:HI 0 "register_operand" "=r")
+ 	(mem:HI (plus:SI
+		  (match_operand:SI 1 "register_operand" "r")
+		  (match_operand:SI 2 "immediate_operand" "i"))))]
+  ""
+  "ldo.s  %0, %2(%1)")
+
+(define_insn "*storehi_offset"
+  [(set (mem:HI (plus:SI
+		  (match_operand:SI 1 "register_operand" "r")
+		  (match_operand:SI 2 "immediate_operand" "i")))
+        (match_operand:HI 0 "register_operand" "r"))]
+  ""
+  "sto.s  %2(%1), %0")

Same here for movhi.


+#ifdef RTX_CODE
+#if defined TREE_CODE
+extern rtx   moxie_function_value (tree, tree, bool ATTRIBUTE_UNUSED);
+#endif
+extern void  moxie_print_operand (FILE *, rtx, int);
+extern void  moxie_print_operand_address (FILE *, rtx);
+extern rtx   moxie_function_arg (CUMULATIVE_ARGS, enum machine_mode, tree, int);
+#endif /* RTX_CODE */

The types "tree" and "rtx" are always defined, by coretypes.h, so you
don't need to protect uses of them with #ifdef.  The only possible issue
here is enum machine_mode.

Ian