This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: PATCH: new port: Moxie


Thanks again Ian.  I've attached a new version of the patch that
addresses your points.  Comments are also embedded in your response
below...


On Sat, 2009-05-02 at 22:31 -0700, Ian Lance Taylor wrote:
> 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")))

Done.


> +(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.

Done.

> 
> +int compute_frame_size (int, long * ATTRIBUTE_UNUSED);
> 
> It seems like this should be static.

This isn't actually used anywhere, so I've deleted the function.


> 
> +/* 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.

Done.


> 
> +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.

Done.


> +  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 '('.

Done, done and done.


> 
> +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.

I left this alone.


> 
> +/* 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?

I've cleaned this up.  Function calls and register elimination were the
two hardest parts of the GCC port.   At one point I had written this
function to put easily identifiable offsets in the code so I could
easily see where they were being used.  Oops - forgot to clean that up.
Only the last two cases above are ever used, so I deleted the rest.


> 
> +/* 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.

All fixed with correct/new comments.


> 
> +#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.

I've changed this to hex constants.


> +/* 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.

All fixed.


> +#define GO_IF_MODE_DEPENDENT_ADDRESS(ADDR,LEBEL)
> 
> Don't define this if there is nothing for it to do.

Ok, deleted.


> +#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.

I've replaced this with builtin_define_std("moxie") and
builtin_define_std("MOXIE").


> > +#define NOTICE_UPDATE_CC(EXPR, INSN) 0
> 
> You shouldn't need to define this.

Ok, deleted.


> 
> +;; 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)

All cleaned up.


> 
> 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.

I don't remember,  but I did.  I'll look into this, but for the purposes
of this patch, I've left this alone.


> +(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?

They were already alternatives of movsi, but the custom predicate
moxie_general_movsrc_operand didn't match the (PLUS ...).  I've tweaked
the predicate code so it matches and deleted all of these define_insns.

> 
> +(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.

Deleted.


> +(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.

Deleted.

> 
> +#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.

Ok, fixed.


> Ian



New patch attached.

Thanks!

AG


Attachment: moxie-gcc-4.patch.gz
Description: GNU Zip compressed data


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]