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: How can I add S+core backend code into GCC package


Hi,

I had a read through the patch and noticed a few potential problems.
I also noticed a lot of formatting nits (i.e. cases where the code
didn't conform to the coding standards).  I know the formatting
comments below are annoyingly niggly, but when you're used to the
gcc coding style, these problems do stand out.  I realise that they
probably don't stand out to you, which is why I've noted each problem
as I've seen it.

liqin@sunnorth.com.cn writes:
> +#FIXME
> +MULTILIB_OPTIONS = mel mSCORE7

Can I put in a plea for this to be changed somehow?  Either by removing
the comment, fixing whatever needs fixing, or saying what might need
to be fixed.  It isn't obvious to an outsider why this line is dubious.

> +(define_predicate "arith_operand"
> +  (match_code "const_int,reg,subreg,const")
> +{
> +  if (GET_CODE (op) == CONST_INT)
> +    return 1;
> +  return register_operand (op, mode);
> +})

A minor nit, but this is better written:

  (define_predicate "arith_operand"
    (ior (match_code "const_int")
         (match_operand 0 "register_operand")))

I realise a lot of other ports use the same style as your code above,
but they were converted automatically from C functions.  Perhaps yours
were too, from an internal port belonging to an earlier version of gcc.
However, the problem with the original form is that it's harder to
maintain the list of match_codes.  E.g. yours above has "const",
but nothing in the C code allows CONST.  Harmless but confusing
(and suboptimal as far as recog() goes).

I see you've used the lispy style in other predicates -- thanks!

> +(define_predicate "call_insn_operand"
> +  (match_code "reg, symbol_ref")
> +{
> +  enum score_symbol_type symbol_type;
> +
> +  if (mda_symbolic_constant_p (op, &symbol_type))
> +     return (symbol_type == SYMBOL_GENERAL);
> +  return register_operand (op, mode);
> +})

A similar problem here, in that mda_symbolic_constant_p accepts more
than just SYMBOL_REF and register_operand accepts more than just REG.
Again it's only a minor nit -- the worst it's going to do is lead to
suboptimal code in unusual cases -- but it might be worth doing
something like:

(define_predicate "general_symbol_operand"
  (match_code "const, symbol_ref, label_ref")
{
  enum score_symbol_type symbol_type;

  return (mda_symbolic_constant_p (op, &symbol_type)
          && symbol_type == SYMBOL_GENERAL);
})

(define_predicate "call_insn_operand"
  (ior (match_operand 0 "register_operand")
       (match_operand 0 "general_symbol_operand")))

> +(define_predicate "hireg_operand"
> +  (and  (match_code "reg")
> +        (match_test "(REGNO (op) == HI_REGNUM)")))
> +
> +(define_predicate "loreg_operand"
> +  (and  (match_code "reg")
> +        (match_test "(REGNO (op) == LO_REGNUM)")))
> +
> +(define_predicate "g32reg_operand"
> +  (and  (match_code "reg")
> +        (match_test "( GP_REG_P (REGNO (op)) )")))

Formatting nits: extra spaces after the "and".  Unncessary outer
parentheses and spaces in the match_tests.

> +(define_predicate "subreg_operand"
> +  (match_code "subreg")
> +{
> +  return ((GET_MODE (op) == QImode || GET_MODE (op) == HImode)
> +           && register_operand (XEXP (op,0), GET_MODE (XEXP (op,0))));
> +})

Formatting nits: spaces after "," in the XEXPs.  However, this predicate
doesn't look right to me.  I'll say why when we get to the pattern that
uses it.

> +enum reg_class score_secondary_reload_class (enum reg_class class, enum machine_mode mode, rtx x);

Another niggly formatting nit (sorry!), but we format for 80-character
terminals.  This line, and others in the file, are too long.

> +void score_function_arg_advance (
> +  CUMULATIVE_ARGS *cum, enum machine_mode mode, tree type, int named);

A trailing "(" isn't conventional.  Either put it next to
CUMULATIVE_ARGS or change the above lines to something like:

void score_function_arg_advance (CUMULATIVE_ARGS *cum, enum machine_mode mode,
                                 tree type, int named);

> +/* How Scalar Function Values Are Returned  */
> +rtx score_function_value (tree valtype, tree func, enum machine_mode mode);
> +
> +/* implementing the Varargs Macro */
> +rtx score_va_arg (tree va_list, tree type);

Comments should start with a capital letter and end with ".  */".
But don't feel compelled to put any comments here; the ones above
the function implementations are enough.

> +(define_expand "movsicc"
> +  [(set (match_operand:SI 0 "register_operand" "")
> +        (if_then_else:SI (match_operand 1 "comparison_operator" "")
> +                         (match_operand:SI 2 "register_operand" "")
> +                         (match_operand:SI 3 "arith_operand" "")))]
> +  ""
> +{
> +  if(mdx_movsicc(operands))
> +    DONE;
> +})

This is confusing.  It implies that mdx_movsicc can return false, and
that if it does return false, gcc should generate the pattern as given
in the expander pattern above, without modifications.  This is not the
case.

As it turns out mdx_movsicc always returns true, so I suggest making
it return no value instead, and changing the C block above to:

{
  mdx_movsicc (operands);
  DONE;
})

(Note spaces before opening parentheses.)

> +(define_insn "movsicc_internal"
> +  [(set (match_operand:SI 0 "register_operand" "=d")
> +        (if_then_else:SI (match_operator 1 "comparison_operator" [(reg:CC CC_REGNUM) (const_int 0)])

Long line.

> +                         (match_operand:SI 2 "arith_operand" "d")
> +                         (match_operand:SI 3 "arith_operand" "0")))]
> +  ""
> +  "mv%C1   %0, %2"
> +  [(set_attr "type" "cndmv")
> +   (set_attr "mode" "SI")])

Why are you using arith_operand here?  It seems that operands 2 and 3
must both be registers.  Using arith_operand isn't wrong as such, but it
will generally lead to worse code.  The problem is that if we need a
temporary register for operands 2 and 3, it won't be introduced until
reload, and the register allocators proper will never see it.

> +(define_insn "zero_extract_bittst"
> +  [(set (reg:CC_NZ CC_REGNUM)
> +        (compare:CC_NZ  (unspec:SI [(match_operand:SI 0 "register_operand" "*e,d")
> +                                    (match_operand:SI 1 "const_bi_operand" "")]
> +                                   BITTST)
> +                        (const_int 0)))]

Excess spacing after CC_NZ (here and elsewhere).

> +(define_insn_and_split "truncate_cmp"
> +  [(set (reg:CC_NZ CC_REGNUM)
> +        (compare:CC_NZ  (zero_extend:SI (match_operand:HI 0 "subreg_operand" "d"))
> +                        (const_int 0)))]
> +  ""
> +  "#"
> +  ""
> +  [(const_int 1)]
> +{
> +  mds_mode_truncate_cmp (operands);
> +    DONE;
> +})

Going back to what I said about subreg_operand, this does not look right.
Your predicate requires a SUBREG while your constraint requires a (general)
REG.  You'll get an ICE if this pattern ever reaches reload.

I see no reason why the operand must be a SUBREG.  I think you should
instead use a normal HImode register_operand here, and rewrite
mds_mode_truncate_cmp to:

void
mds_mode_truncate_cmp (rtx *ops)
{
  unsigned HOST_WIDE_INT mask;
  mask = GET_MODE (ops[0]) == QImode ? 0xff: 0xffff;
  emit_insn (gen_andsi3_cmp (gen_lowpart (SImode, ops[0]), GEN_INT (mask))));
}

Or you could just inline it in the define_insn_and_split,
as it's pretty simple.

You can then ditch subreg_operand.

But this still looks a little strange.  Your pattern requires the
subreg_operand to be HImode, and it's the only thing to use subreg_operand
and mds_mode_truncate_cmp.  However, subreg_operand and mds_mode_truncate_cmp
are both geared to support QImode as well as HImode.  If you do want to
support both modes, you need a separate insn for QImode.  Mode macros
can help here:

(define_mode_macro SHORT [QI HI])

(define_insn_and_split "*truncate_cmp<mode>"
  [(set (reg:CC_NZ CC_REGNUM)
        (compare:CC_NZ (zero_extend:SI
                         (match_operand:SHORT 0 "register_operand" "d"))
                       (const_int 0)))]
  ""
  "#"
  ""
  [(const_int 1)]
{
  mds_mode_truncate_cmp (operands);
  DONE;
})

> +(define_insn_and_split "*truncate_hi_ucc"
> +  [(set (reg:CC_NZ CC_REGNUM)
> +        (compare:CC_NZ  (and:SI (match_operand:SI 1 "register_operand" "0")
> +                                (match_operand:SI 2 "const_int_operand" ""))
> +                        (const_int 0)))
> +   (set (match_operand:SI 0 "register_operand" "=d")
> +        (zero_extend:SI (match_operand:HI 3 "register_operand" "0")))]
> +  "INTVAL (operands[2]) == 0xffff"
> +  "#"
> +  ""
> +  [(const_int 1)]
> +{
> +  mds_mode_truncate_ucc (operands);
> +    DONE;
> +})

This pattern presents operands 1 and 3 as separate inputs.  As far as
the optimisers are concerned, there's no reason why the operands must
hold the same value.  The natural thing would be to make operand 3
a match_dup, but of course, that won't work here because the register
is being accessed in different modes.

This raises the question: why does the pattern have to represent the
same operation in two different ways?  Are you sure this pattern ever
matches?  If so, this sounds like a bug in the target-independent
canonicalisation code.

Same comments apply to...

> +(define_insn_and_split "*truncate_qi_ucc"

...this.

> +(define_peephole
> +  [(set (match_operand:VOID 0 "register_operand" "=d")
> +        (zero_extend:SI (match_operand:VOID 1 "memory_operand" "")))
> +   (set (match_operand:SI 2 "register_operand" "=d")
> +        (plus:SI (match_dup 2)
> +                 (match_operand:SI 3 "pindex_off_operand" "")))]
> +  "GET_MODE_SIZE (GET_MODE (operands[0])) <= GET_MODE_SIZE (HImode) &&
> +   GET_MODE (operands[1]) == GET_MODE (operands[0]) &&
> +   register_operand (XEXP (operands[1], 0), SImode) &&
> +   REGNO (XEXP (operands[1], 0)) == REGNO (operands[2])"
> +{
> +  switch (GET_MODE (operands[0]))
> +    {
> +    case QImode: return \"lbu     %0, [%2]+, %c3\";
> +    case HImode: return \"lhu     %0, [%2]+, %c3\";
> +    default: abort();
> +    }
> +}
> +  [(set_attr "type" "load")])

Can you convert these define_peephols into a define_peephole2s?
define_peephole is deprecated.  (I've skimmed over them in the
hope that the answer is "yes" ;)

Also, "&&" should be on the start of the new line, not the end
of the previous line.

> +  "!TARGET_SCORE5U && !TARGET_LIT_ENDIAN"

Can I put in a plea for this macro to be called TARGET_LITTLE_ENDIAN?
Many ports use that spelling and none use TARGET_LIT_ENDIAN.  (I tend
to assuem "lit" stands for something like "literal".)

> +(define_insn "move_scb"
> +  [(set (match_operand:SI 0 "register_operand" "=d")
> +        (plus:SI (match_operand:SI 1 "register_operand" "0")
> +                 (const_int 4)))
> +   (set (mem:SI (match_dup 1))
> +        (unspec:SI [(match_operand:SI 2 "register_operand" "d")] SCB))
> +   (set (reg:SI SC_REGNUM)
> +        (unspec:SI [(match_dup 2)] SCLC))]
> +  "!TARGET_SCORE5U"
> +  "scb     %2, [%1]+"
> +  [(set_attr "type" "store")
> +   (set_attr "mode" "SI")])

It's fine to use (mem: ...) patterns to match existing insns, but it's
a bad idea to generate them (i.e. to call gen_move_scb).  The problem
is that the MEM will have no aliasing information, etc.

I haven't read the ISA manual, but I see that an uanligned store expands
to an scb followed by an sce.  I assume that scb and sce are therefore
partial stores, similar in spirit to MIPS's swl and swr.  Is that right?

If so, something is wrong here.  You're expanding a 4-byte unaligned
store into what looks to the rtl optimisers like an 8-byte store of
completely new data (the 4 byte store above and then another store to
the new value of operand 0 in move_sce).  GCC may well delete earlier
stores to the same 8-byte region as dead.

Also, SImode MEMs must always be aligned on STRICT_ALIGNMENT targets,
even those created by the backend itself.  If you want an unaligned
4-byte MEM, you should create a BLKmode memory reference and set
its size to 4 bytes using set_mem_size.

> +(define_insn "move_scw"
> +  [(set (match_operand:SI 0 "register_operand" "=d")
> +        (plus:SI (match_operand:SI 1 "register_operand" "0")
> +                 (const_int 4)))
> +   (set (mem:SI (match_dup 1))
> +        (unspec:SI [(match_operand:SI 2 "register_operand" "d")
> +                    (reg:SI SC_REGNUM)]
> +                   SCW))
> +   (set (reg:SI SC_REGNUM)
> +        (unspec:SI [(match_dup 2)] SCLC))]
> +  "!TARGET_SCORE5U"
> +  "scw     %2,[%1]+"
> +  [(set_attr "type" "store")
> +   (set_attr "mode" "SI")])

Again making assumptions from reading the code, I'm guessing that scw
stores a full word to (%1 & -3).  If so, the address should say that:

    (and (match_dup 1)
         (const_int -3))

> +(define_insn "move_lcb"
> +  [(set (match_operand:SI 0 "register_operand" "=d")
> +        (plus:SI (match_operand:SI 1 "register_operand" "0")
> +                 (const_int 4)))
> +   (set (reg:SI LC_REGNUM)
> +        (unspec:SI [(mem:SI (match_dup 1))] LCB))]
> +  "!TARGET_SCORE5U"
> +  "lcb     [%1]+"
> +  [(set_attr "type" "load")
> +   (set_attr "mode" "SI")])

The move_lc[bwe] group suffers from the same sort of problems as
move_sc[bwe].

> +/* Implement TARGET_RETURN_IN_MEMORY.  In S+core,
> +   small structures are returned in a register.
> +   Objects with varying size must still be returned in memory,
> +   of course.  */
> +static bool
> +score_return_in_memory (tree type, tree fndecl ATTRIBUTE_UNUSED)
> +{
> +  return ((TYPE_MODE (type) == BLKmode) || (int_size_in_bytes (type) > (2 * UNITS_PER_WORD))
> +          || (int_size_in_bytes (type) == -1));
> +}

Long lines & unnecessary parentheses.

> +/* Return nonzero when an argument must be passed by reference.  */
> +static bool
> +score_pass_by_reference (CUMULATIVE_ARGS *cum ATTRIBUTE_UNUSED,
> +                         enum machine_mode mode, tree type,
> +                         bool named ATTRIBUTE_UNUSED)
> +{
> +  /* If we have a variable-sized parameter, we have no choice.  */
> +  return targetm.calls.must_pass_in_stack (mode, type);
> +}
> +
> +/* Return a legitimate address for REG + OFFSET.  */
> +static rtx
> +score_add_offset (rtx temp, rtx reg, HOST_WIDE_INT offset)
> +{
> +  if (!CONST_OK_FOR_LETTER_P (offset, 'O'))
> +    {
> +      reg = expand_simple_binop (GET_MODE (reg), PLUS,
> +                                 GEN_INT (offset & 0xffffc000),
> +                                 reg, NULL, 0, OPTAB_WIDEN);
> +      offset &= 0x3fff;
> +    }

GEN_INT (.... & 0xffffc000) isn't right for 64-bit HOST_WIDE_INTs.
Integers must always be stored in sign-extended form.  Use

    gen_int_mode (offset & 0xffffc000, ...)

instead.

> +  /* Jump to the target function.  Use a sibcall if direct jumps are
> +     allowed, otherwise load the address into a register first.  */
> +  fnaddr = XEXP (DECL_RTL (function), 0);
> +  insn = emit_call_insn (gen_sibcall_internal (fnaddr, const0_rtx));
> +  SIBLING_CALL_P (insn) = 1;

The comment needs updating.  You always use a direct sibling call here.

> +static bool
> +th_strict_argument_naming (CUMULATIVE_ARGS* ca ATTRIBUTE_UNUSED)
> +{
> +  return true;
> +}

Just a personal request -- feel free to ignore -- but could you stick a
comment above each hook like this to say what it's implementing?
Something like "/* Implement TARGET_STRICT_ARGUMENT_NAMING.  */"?
It's then obvious which functions are hook implementations and which
are internal to the backend.

However, you could just use hook_bool_CUMULATIVE_ARGS_true for this hook.

> +static bool
> +th_function_ok_for_sibcall (ATTRIBUTE_UNUSED tree decl,
> +                            ATTRIBUTE_UNUSED tree exp)
> +{
> +  return true;
> +}

Could be hook_bool_tree_tree_true.  We only have hook_bool_tree_tree_true
at the moment, but feel free to add others like this.  Just a minor nit
though.

> +  /* The offset of the first register from GP_ARG_FIRST or FP_ARG_FIRST,
> +     or ARG_REG_NUM if the argument is passed entirely on the stack.  */
> +  unsigned int reg_offset;

This comment doesn't make sense for this port.  GP_ARG_FIRST and
FP_ARG_FIRST are not defined.

> +static bool
> +th_target_return_in_msb (tree valtype ATTRIBUTE_UNUSED)
> +{
> +  return 0;
> +}

bool_hook_tree_false

> +/* Fill INFO with information about a single argument.  CUM is the
> +   cumulative state for earlier arguments.  MODE is the mode of this
> +   argument and TYPE is its type (if known).  NAMED is true if this
> +   is a named (fixed) argument rather than a variable one.  */
> +
> +static void
> +classify_arg (const CUMULATIVE_ARGS *cum, enum machine_mode mode,
> +              tree type, int named, struct score_arg_info *info)
> +{
> +  int even_reg_p;
> +  unsigned int num_words, max_regs;
> +
> +  even_reg_p = 0;
> +  if (GET_MODE_CLASS (mode) == MODE_INT ||
> +      GET_MODE_CLASS (mode) == MODE_FLOAT)

Formatting.

> +      info->reg_offset =(cum->num_gprs);

Likewise.

> +  num_words =(info->num_bytes + UNITS_PER_WORD - 1) / UNITS_PER_WORD;

Likewise.

> +  struct score_frame_info* f = mda_cached_frame ();

Space before the "*" rather than after it.

> +               (reg_names[(frame_pointer_needed) ? HARD_FRAME_POINTER_REGNUM : STACK_POINTER_REGNUM]),

Long line.

> +               current_function_is_leaf?1: 0,

Formatting.

> +static int
> +th_issue_rate (void)
> +{
> +        return 1;
> +}

Likewise.

> +static void
> +th_select_section (tree decl, int reloc,
> +                   unsigned HOST_WIDE_INT align)
> +{
> +  if (targetm.have_named_sections)
> +    default_elf_select_section (decl, reloc, align);
> +  else
> +    default_select_section (decl, reloc, align);
> +}

I don't understand the need for this.  Do you have an score target
without named sections?  I would have expected the default to work.

> +/* Returns nonzero if X contains a SYMBOL_REF.  */
> +static int
> +symbolic_expression_p (rtx x)

Minor nit, but please use bool rather than int for booleans.
(And "true" rather than "1", "false" rather than "0".)

> +  if (GET_CODE(x) == CONST)
> +    return symbolic_expression_p(XEXP(x, 0));

Formatting.

> +  if (GET_RTX_CLASS (GET_CODE (x)) == '1')
> +    return symbolic_expression_p (XEXP (x, 0));
> +
> +  if (GET_RTX_CLASS (GET_CODE (x)) == 'c'
> +      || GET_RTX_CLASS (GET_CODE (x)) == '2')
> +    return (symbolic_expression_p (XEXP (x, 0))
> +            || symbolic_expression_p (XEXP (x, 1)) );

If I've understood correctly, this code is destined for 4.2.
Is that right?  If so, the class checks above need to be updated.
We no longer use those horrible special characters (thanks Paolo!)
We also have predicates like UNARY_P now, see rtl.h for detais.

> +  else if (flag_pic && symbolic_expression_p (x))
> +        return get_named_section (0, ".data.rel.ro", 3);

Excess space.

> +/* Implement TARGET_IN_SMALL_DATA_P.  Return true if it would be safe to
> +   access DECL using %gp_rel(...)($gp).  */

%gp_rel(...)($gp) is MIPS syntax, not score syntax.

> +    fprintf(asm_out_file, "\t.set pic\n");

Formatting.

> +/* Implement TARGET_ASM_FILE_END.  When using assembler macros, emit
> +   .externs for any small-data variables that turned out to be external.  */

Do you have assembler macros on score?  Do you actually need these
.extern directives?  (I'd hope not!  It's a horrible wart on MIPS.)

> +static unsigned int sdata_max;
> +
> +int
> +score_sdata_max (void)
> +{
> +  return sdata_max;
> +}

(and "#define SCORE_SDATA_MAX score_sdata_max ()" in score.h).
I'm not sure I understand the reason for this.  Is it from a
"don't expose variables in header files" philosophy?  If so,
I wouldn't worry. ;)  gcc is a lost cause if you care about that.
You might as well just call the variable score_sdata_max and
avoid the macro and function.

> +int reg_class_mask[][5] = REG_CLASS_CONTENTS;
> +
> +int
> +score_reg_class (int regno)
> +{
> +  int c;
> +  err_assert (regno >= 0 && regno < FIRST_PSEUDO_REGISTER);
> +
> +  if (regno == FRAME_POINTER_REGNUM
> +      || regno == ARG_POINTER_REGNUM)
> +    return ALL_REGS;
> +
> +  for (c = 0 ; c < N_REG_CLASSES ; c ++)
> +    if (reg_class_mask[c][regno/32] & (1 << regno))
> +      return c;
> +  return NO_REGS;
> +}

Rather than duplicate REG_CLASS_CONTENTS, you can just use:

  if (TEST_HARD_REG_BIT (reg_class_contents[c], regno))

> +/* imm constraints
> +   I        IMM8        (i15-2-form)
> +   J        IMM5        (i15_1-form)
> +   K        IMM16          (i-form        )
> +   L        IMM16s         (i-form        )
> +   M        IMM14        (ri-form)
> +   N        IMM14s        (ri-form)
> +   O    IMM15s        (ri-form)
> +   P        IMM12s         (rix-form) / IMM10s(cop-form) << 2
> +   Q    const_hi imm
> +   Z    SYMBOL_REF */

The formatting of this table is a little odd.

'Z'/SYMBOL_REF is not an immediate constraint.

> +int
> +score_const_ok_for_letter_p (int value, char c)
> +{
> +  switch (c)
> +    {
> +    case 'I': return IMM_IN_RANGE (value,  8, 0);
> +    case 'J': return IMM_IN_RANGE (value,  5, 0);
> +    case 'K': return IMM_IN_RANGE (value, 16, 0);
> +    case 'L': return IMM_IN_RANGE (value, 16, 1);
> +    case 'M': return IMM_IN_RANGE (value, 14, 0);
> +    case 'N': return IMM_IN_RANGE (value, 14, 1);
> +    case 'O': return IMM_IN_RANGE (value, 15, 1);
> +    case 'P': return IMM_IN_RANGE (value, 12, 1);
> +    case 'Q': return score_extra_constraint (GEN_INT (value), c);

'Q' is not an immediate constraint, so the last line does not belong here.
What you do in score_extra_constraint is fine though.

I notice that you use CONST_OK_FOR_LETTER_P (... 'Q'), but I'll come
to that later.

> +    default : return 0;
> +    }
> +}

FYI, these can now be handled by define_constraints in the .md file.
However, that's a new feature, and many ports have not been converted,
so I don't think there's any reason why you need to convert either.

> +int
> +score_extra_constraint (rtx op, char c)
> +{
> +  switch (c)
> +    {
> +    case 'Q':
> +      return (GET_CODE (op) == CONST_INT && (INTVAL (op) & 0xffff) == 0);
> +    case 'W':
> +      if (GET_CODE (op) == CONST_INT)
> +        return !INTVAL (op);
> +      return false;

There's an odd clash of coding style in these two cases.

> +          XVECEXP (ret, 0, i) = gen_rtx_EXPR_LIST (SImode, reg, GEN_INT (part_offset));

Long line.

> +  unsigned int tramp[TRAMPOLINE_INSNS] = {
> +                0x8103bc56,                         /* mv      r8, r3          */
> +                0x9000bc05,                         /* bl      0x0x8           */
> +                0xc1238000 | (CODE_SIZE - 8),       /* lw      r9, &func       */
> +                0xc0038000 | (STATIC_CHAIN_REGNUM << 21) | (CODE_SIZE - 4),   /* lw  static chain reg, &chain */
> +                0x8068bc56,                         /* mv      r3, r8          */
> +                0x8009bc08,                         /* br      r9              */
> +                0x0,
> +                0x0,
> +        };

Odd identation.

> +int
> +score_address_p (enum machine_mode mode, rtx x, int strict)
> +{
> +  struct score_address_info addr;
> +  enum rtx_code code = GET_CODE(x);
> +
> +  return mda_classify_address (&addr, mode, x, strict);
> +}

"code" isn't used.

> +#define GR_REG_CLASS_P(C)        ((C) == G16_REGS || (C) == G32_REGS)
> +#define SP_REG_CLASS_P(C)        ((C) == CN_REG || (C) == LC_REG || \
> +                                  (C) == SC_REG || (C) == SP_REGS)
> +#define CP_REG_CLASS_P(C)        ((C) == CP1_REGS || (C) == CP2_REGS || \
> +                                  (C) == CP3_REGS || (C) == CPA_REGS )
> +#define CE_REG_CLASS_P(C)        ((C) == HI_REG || (C) == LO_REG || (C) == CE_REGS)

You might as well use GR_REG_CLASS_P in score_secondary_reload_class too,
to replace:

+  if (!(class == G32_REGS || class == G16_REGS))

> +/* When using assembler macros, keep track of all of small-data externs
> +   so that th_asm_file_end can emit the appropriate declarations for them.
> +
> +   In most cases it would be safe (though pointless) to emit .externs
> +   for other symbols too.  One exception is when an object is within
> +   the -G limit but declared by the user to be in a section other
> +   than .sbss or .sdata.  */
> +int
> +score_output_external (FILE *file ATTRIBUTE_UNUSED,
> +                       tree decl, const char *name)
> +{
> +  register struct extern_list *p;
> +
> +  if (th_in_small_data_p (decl))
> +    {
> +      p = (struct extern_list *)ggc_alloc (sizeof (struct extern_list));
> +      p->next = extern_head;
> +      p->name = name;
> +      p->size = int_size_in_bytes (TREE_TYPE (decl));
> +      extern_head = p;
> +    }
> +  return 0;
> +}

Same question as above about whether this is really needed for your port.

> +  else if (c == 'U')
> +    {
> +      err_assert (code == CONST_INT);
> +      fprintf (file, "0x%lx", (unsigned HOST_WIDE_INT)INTVAL (op) >> 16);
> +    }

Format/argument mismatch.  Use HOST_WIDE_INT_PRINT_HEX or cast to
a different type.

> +        fprintf (file, "0x%08lx", TARGET_LIT_ENDIAN ?
> +                 CONST_DOUBLE_LOW (op):
> +                 CONST_DOUBLE_HIGH (op));

Likewise.

> +  else if(c == 'V')

Formatting.

> +  else if (code == REG || code == SUBREG)
> +    {
> +      int regnum = (code == REG) ? (int)REGNO (op) : true_regnum (op);

Likewise.

You should not see SUBREGs here.  Only the REG/REGNO code should be needed.

> +        default:  fatal_insn ("PRINT_OPERAND, invalid insn for %%C", op);

Unfortunately, you chose a bad example in the MIPS port here ;)
The right function to use is output_operand_lossage.  Fixing the
MIPS port is on my to-do list.

> +  else{
> +    switch (code)

Formatting: no need for this {.

> +      default:
> +        output_addr_const (file, op);
> +        /* print_rtl (stderr, op);
> +           fprintf (stderr, "\n");
> +           err_assert (0);  */

It isn't obvious why you'd ever want to enable the commented-out code.
Maybe just remove it?  output_addr_const is bound to complain if you
pass it something it doesn't like.

> +score_print_operand_address (FILE *file, rtx x)
> +{
> +  struct score_address_info addr;
> +  enum rtx_code code = GET_CODE (x);
> +  enum machine_mode mode = GET_MODE (x);
> +
> +  if (code == MEM)
> +    x = XEXP (x, 0);

I'd recommend against this.  Callers should always pass the address,
not the containing MEM.  As things stand, it looks like you're allowing
memory indirect addressing while silently ignoring the mem.

> Index: gcc/config/score/elf.h
> ===================================================================
> --- gcc/config/score/elf.h	(revision 0)
> +++ gcc/config/score/elf.h	(revision 0)

I think you can get rid of some parts of this file if you use
config/elfos.h, but that's just a minor cleanup.

> +(define_insn "smaxsi3"
> +  [(set (match_operand:SI 0 "register_operand" "=d")
> +        (smax:SI (match_operand:SI 1 "arith_operand" " d")
> +                 (match_operand:SI 2 "arith_operand" " d")))]
> +  "TARGET_MAC"
> +  "max     %0, %1, %2"
> +  [(set_attr "type" "arith")
> +   (set_attr "mode" "SI")])

Same arith_operand comment as for movsicc.  Also for other patterns
in this file; I won't list each one.

> +(define_insn "sffs"
> +  [(set (match_operand:SI 0 "register_operand" "=d")
> +        (unspec:SI [(match_operand:SI 1 "register_operand" "d")] 33))]
> +  "TARGET_MAC"
> +  "bitrev  %0, %1, r0\;clz     %0, %0\;addi    %0, 0x1"
> +  [(set_attr "type" "arith")
> +   (set_attr "mode" "SI")])

You've done the right thing and used define_constants elsewhere.
Please use them for this UNSPEC too ;)

> +(define_expand "ffssi2"
> +  [(set (match_operand:SI 0 "register_operand" "=d")
> +        (ffs:SI (match_operand:SI 1 "register_operand" " d")))]

Constraints in expanders have no meaning.

> +  "TARGET_MAC"
> +{
> +  emit_insn (gen_sffs (operands[0], operands[1]));
> +  emit_insn (gen_rtx_SET (VOIDmode, gen_rtx_REG (CC_NZmode, CC_REGNUM),
> +                          gen_rtx_COMPARE (CC_NZmode, operands[0], GEN_INT (33))));

Long line.

> +(define_peephole2
> +  [(set (match_operand:SI 0 "loreg_operand" "")
> +        (match_operand:SI 1 "arith_operand" ""))
> +   (set (match_operand:SI 2 "hireg_operand" "")
> +        (match_operand:SI 3 "arith_operand" ""))]
> +  "TARGET_MAC"
> +  [(parallel
> +       [(set (match_dup 0) (match_dup 1))
> +        (set (match_dup 2) (match_dup 3))])])
> +
> +(define_peephole2
> +  [(set (match_operand:SI 0 "hireg_operand" "")
> +        (match_operand:SI 1 "arith_operand" ""))
> +   (set (match_operand:SI 2 "loreg_operand" "")
> +        (match_operand:SI 3 "arith_operand" ""))]
> +  "TARGET_MAC"
> +  [(parallel
> +       [(set (match_dup 2) (match_dup 3))
> +        (set (match_dup 0) (match_dup 1))])])
> +
> +(define_insn "movtohilo"
> +  [(parallel
> +       [(set (match_operand:SI 0 "loreg_operand" "")
> +             (match_operand:SI 1 "register_operand" ""))
> +        (set (match_operand:SI 2 "hireg_operand" "")
> +             (match_operand:SI 3 "register_operand" ""))])]
> +  "TARGET_MAC"
> +  "mtcehl  %3, %1"
> +  [(set_attr "type" "fce")
> +   (set_attr "mode" "SI")])

I think you mean register_operand rather than arith_operand in those
define_peephole2s.  The define_insn needs constraints.

> +(define_insn "mulsidi3adddi"
> +  [(set (match_operand:DI 0 "register_operand" "=x")
> +        (plus:DI (mult:DI (sign_extend:DI (match_operand:SI 2 "arith_operand" "%d"))
> +                          (sign_extend:DI (match_operand:SI 3 "arith_operand" "d")))

You really should use register_operand in these sorts of patterns.
It doesn't make sense to have (sign_extend:DI (const_int ...)).
Again, I won't list each pattern individually.

> +/* Identification of SUNPLUS S+CORE Port.  */
> +#ifndef SUNPLUS
> +#define SUNPLUS
> +#endif
> +#ifndef SCORE
> +#define SCORE
> +#endif

What are these defines for?

> +/* Controlling the Compilation Driver.  */
> +#define SWITCH_TAKES_ARG(CHAR)        (DEFAULT_SWITCH_TAKES_ARG(CHAR) || (CHAR) == 'G')

Formatting and long line.

> +#define CC1_SPEC                 "%{!mel:-meb} %{mel:-mel} "

The last bit is superfluous.  All -m* options are passed to cc1.

> +#define ASM_SPEC                 "%{!mel:-EB} %{mel:-EL} %{mSCORE5U:-SCORE5U}  %{mSCORE7:-SCORE7} \

Long line.

> +      if(TARGET_SCORE5U)                        \
> +                builtin_define("__score5u__");  \

Formatting.

> +#define TARGET_VERSION                          fprintf(stderr, "Sunplus S+CORE %s", SCORE_GCC_VERSION);

Formatting and long line.

> +#define OVERRIDE_OPTIONS                        score_override_options()

Formatting.

> +/* Define this to set the endianness to use in libgcc2.c, which can
> +   not depend on target_flags.  */
> +#if defined __scorele__
> +        #define LIBGCC2_WORDS_BIG_ENDIAN        0
> +#else
> +        #define LIBGCC2_WORDS_BIG_ENDIAN        1
> +#endif

Excess indentation.

> +/* Define this macro if it is advisable to hold scalars in registers
> +   in a wider mode than that declared by the program.  In such cases,
> +   the value is constrained to be within the bounds of the declared
> +   type, but kept valid in the wider mode.  The signedness of the
> +   extension may differ from that of the type.  */
> +#define PROMOTE_MODE(MODE, UNSIGNEDP, TYPE)                 \
> +                if (GET_MODE_CLASS (MODE) == MODE_INT       \
> +                   && GET_MODE_SIZE (MODE) < UNITS_PER_WORD)\
> +                        (MODE) = SImode;

Formatting.

> +/* If defined, a C expression to compute the alignment given to a
> +   constant that is being placed in memory.  CONSTANT is the constant
> +   and ALIGN is the alignment that the object would ordinarily have.
> +   The value of this macro is used instead of that alignment to align
> +   the object.
> +
> +   If this macro is not defined, then ALIGN is used.
> +
> +   The typical use of this macro is to increase alignment for string
> +   constants to be word aligned so that `strcpy' calls that copy
> +   constants can be done inline.  */

I know you're copying from existing ports, but it's now considered
bad practice to cut-&-paste the internal documentation into the
backend code.  The problem is that they tend to get out of sync.
You should really just say why your definition is right for score.
In some cases, the reason is blindingly obvious, and no comment
is needed.

I've picked this comment as a random example.  The same thing
applies elsewhere.

> +#define HARD_REGNO_NREGS(REGNO, MODE)  ((GET_MODE_SIZE(MODE)+UNITS_PER_WORD-1)/UNITS_PER_WORD)

Formatting and long line.

> +/* Value is 1 if it is a good idea to tie two pseudo registers
> +   when one has mode MODE1 and one has mode MODE2.
> +   If HARD_REGNO_MODE_OK could produce different values for MODE1 and MODE2,
> +   for any hard reg, then this must be 0 for correct output.  */
> +
> +#define MODES_TIEABLE_P(MODE1, MODE2)                                   \
> +        ((GET_MODE_CLASS(MODE1) == MODE_FLOAT ||                        \
> +                GET_MODE_CLASS(MODE1) == MODE_COMPLEX_FLOAT) ==         \
> +        (GET_MODE_CLASS(MODE2) == MODE_FLOAT ||                         \
> +                GET_MODE_CLASS(MODE2) == MODE_COMPLEX_FLOAT))

Formatting.  (This is again badness in the MIPS port. ;))

> +enum reg_class{
> +        NO_REGS,
> +        G16_REGS,               /* r0 ~ r15 */
> +        G32_REGS,               /* r0 ~ r31 */
> +        T32_REGS,               /* r8 ~ r11 | r22 ~ r27 */
> +
> +        HI_REG,                 /* hi                 */
> +        LO_REG,                 /* lo                 */
> +        CE_REGS,                /* hi + lo            */
> +
> +        CN_REG,                 /* cnt                */
> +        LC_REG,                 /* lcb                */
> +        SC_REG,                 /* scb                */
> +        SP_REGS,                /* cnt + lcb + scb    */
> +
> +        CR_REGS,                /* cr0 - cr15         */
> +
> +        CP1_REGS,               /* cp1                */
> +        CP2_REGS,               /* cp2                */
> +        CP3_REGS,               /* cp3                */
> +        CPA_REGS,               /* cp1 + cp2 + cp3    */
> +
> +        ALL_REGS,
> +        LIM_REG_CLASSES
> +};

Formatting.

> +#define N_REG_CLASSES                 ((int)LIM_REG_CLASSES)

Likewise.

> +        score_regno_mode_ok_for_base_p(REGNO, 1)

Likewise.

> +        score_preferred_reload_class(X, CLASS)

Likewise.

> +        score_secondary_reload_class(CLASS, MODE, X)

Likewise.

> +        score_secondary_reload_class(CLASS, MODE, X)

Likewise.

> +        ((GET_MODE_SIZE(MODE) + UNITS_PER_WORD - 1)/UNITS_PER_WORD)

Likewise.

> +        (GET_MODE_SIZE(FROM) != GET_MODE_SIZE(TO) ? \
> +         reg_classes_intersect_p (HI_REG, (CLASS)): 0)

Likewise.

> +#define CONST_OK_FOR_LETTER_P(VALUE, C)   score_const_ok_for_letter_p(VALUE,C)

Likewise.

> +#define CONST_DOUBLE_OK_FOR_LETTER_P(VALUE, C)        \
> +        ((C) == 'G' && (VALUE) == CONST0_RTX(GET_MODE(VALUE)))

Likewise.

> +#define INCOMING_RETURN_ADDR_RTX        gen_rtx_REG(VOIDmode, RA_REGNUM)

Likewise.

> +#define EH_RETURN_STACKADJ_RTX          gen_rtx_REG(Pmode, EH_REGNUM)

Likewise.

> +   In S+core, we need a frame pointer for a large frame; otherwise,
> +   reload may be unable to compute the address of a local variable,
> +   since there is no way to add a large constant to the stack pointer
> +   without using a temporary register.  */

Where is this requirement enforced?

Also remember that the stack can grow between each elimination attempt.

> +#define CAN_ELIMINATE(FROM, TO)                                 \
> +        (((TO) == HARD_FRAME_POINTER_REGNUM) ||                 \
> +         ((TO) == STACK_POINTER_REGNUM && !frame_pointer_needed))

Formatting.

> +#define INITIAL_ELIMINATION_OFFSET(FROM, TO, OFFSET)            \
> +        (OFFSET) = score_initial_elimination_offset((FROM), (TO))

Likewise.

> +        score_function_arg(&CUM, MODE, TYPE, NAMED)

Likewise.

> +typedef struct score_args{
> +        unsigned int arg_number;                /* how many arguments have been seen  */
> +        unsigned int num_gprs;                  /* number of gprs in use  */
> +        unsigned int stack_words                /* number of words in stack  */
> +}score_args_t;

Likewise.

> +        score_init_cumulative_args(&CUM, FNTYPE, LIBNAME)

Likewise.

> +        score_function_arg_advance(&CUM, MODE, TYPE, NAMED)

Likewise.

> +        REG_CONTAIN(REGNO, ARG_REG_FIRST, ARG_REG_NUM)

Likewise.

> +        score_function_value((VALTYPE), (FUNC), VOIDmode)

Likewise.

> +#define LIBCALL_VALUE(MODE)  score_function_value(NULL_TREE, NULL, (MODE))

Likewise.

> +#define FUNCTION_PROFILER(FILE, LABELNO)                                \
> +{                                                                       \
> +        fprintf(FILE, ".set r1  \n");                                   \
> +        fprintf(FILE, " mv  r%d,r%d \n", AT_REGNUM, RA_REGNUM);         \
> +        fprintf(FILE, " subi r%d, %d \n", STACK_POINTER_REGNUM, 8);     \
> +        fprintf(FILE, " jl   _mcount \n");                              \
> +        fprintf(FILE, ".set nor1 \n");                                  \
> +}

Likewise.

> +#define TRAMPOLINE_SIZE                 ((TRAMPOLINE_INSNS * GET_MODE_SIZE(SImode)) + \
> +                                        (GET_MODE_SIZE(ptr_mode) * 2))

Likewise.

> +        score_initialize_trampoline(ADDR, FUNC, CHAIN)

Likewise.

> +#define CONSTANT_ADDRESS_P(X)           CONSTANT_P(X)

Likewise.

> +#ifdef REG_OK_STRICT
> +#define GO_IF_LEGITIMATE_ADDRESS(MODE, X, LABEL)   \
> +        if(score_address_p(MODE, X, 1)) \
> +                goto LABEL;
> +#else
> +#define GO_IF_LEGITIMATE_ADDRESS(MODE, X, LABEL)   \
> +        if(score_address_p(MODE, X, 0)) \
> +                goto LABEL;
> +#endif

Likewise.

> +#ifndef REG_OK_STRICT
> +        #define REG_MODE_OK_FOR_BASE_P(X, MODE) \
> +                score_regno_mode_ok_for_base_p(REGNO(X), 0)
> +#else
> +        #define REG_MODE_OK_FOR_BASE_P(X, MODE) \
> +                score_regno_mode_ok_for_base_p(REGNO(X), 1)
> +#endif

Likewise.  Exccess indentation.

> +          do{                                           \
> +                if(score_legitimize_address(&(X)))      \
> +                        goto WIN;                       \
> +        }while(0)

Formatting.

> +        score_register_move_cost(MODE, FROM, TO)

Likewise.

> +#define MEMORY_MOVE_COST(MODE,CLASS,TO_P)         \
> +        ((4) + memory_move_secondary_cost((MODE), (CLASS), (TO_P)))

Likewise.  (Including redundant parentheses around the "4".)

> +        score_declare_object(STREAM, NAME, "\n\t.comm\t",                        \
> +                                         ","HOST_WIDE_INT_PRINT_UNSIGNED",%u\n", \
> +                                        SIZE, ALIGN / BITS_PER_UNIT);

Formatting.

> +         score_declare_object(STREAM, NAME, "\n\t.lcomm\t",                      \
> +                                         ","HOST_WIDE_INT_PRINT_UNSIGNED",%u\n", \
> +                                        SIZE, ALIGN / BITS_PER_UNIT);

Likewise.

> +        score_declare_object(STREAM, NAME, "", ":\n", 0)

Likewise.

> +        score_output_external(STREAM,DECL,NAME)

Likewise.

> +        fprintf((STREAM), "%s", (NAME))

Likewise.

> +        sprintf((LABEL), "*%s%s%ld", (LOCAL_LABEL_PREFIX), (PREFIX), (long)(NUM))

Likewise.  Long line.

> +#define PRINT_OPERAND(STREAM, X, CODE)    score_print_operand(STREAM, X, CODE)

Formatting.

> +#define PRINT_OPERAND_ADDRESS(STREAM, X)  score_print_operand_address(STREAM, X)

Likewise.

> +        do{                                                     \
> +                 fprintf(STREAM, "\tpush! %s,[%s]\n",           \
> +                         reg_names[REGNO],                      \
> +                         reg_names[STACK_POINTER_REGNUM]);      \
> +        }while(0)

Likewise.

> +#define ASM_OUTPUT_REG_POP(STREAM,REGNO)                        \
> +        do{                                                     \
> +                 fprintf(STREAM, "\tpop! %s,[%s]\n",            \
> +                         reg_names[REGNO],                      \
> +                         reg_names[STACK_POINTER_REGNUM]);      \
> +        }while(0)

Likewise.

> +do {                                                            \
> +  if (flag_pic)                                                 \
> +    fprintf(STREAM, "\t.gpword %sL%d\n", LOCAL_LABEL_PREFIX, VALUE); \
> +  else                                                               \
> +    fprintf(STREAM, "\t.word %sL%d\n", LOCAL_LABEL_PREFIX, VALUE);   \
> +} while (0)

Likewise.

> +        fprintf(STREAM, "\t.word %sL%d\n", LOCAL_LABEL_PREFIX, VALUE)

Likewise.

> +#define MASK_RETURN_ADDR                 GEN_INT(-1)

You might as well use constm1_rtx.

> +        fprintf(STREAM, "\t.space\t"HOST_WIDE_INT_PRINT_UNSIGNED"\n", (SIZE))

Formatting.

> +        fprintf(STREAM, "\t.align\t%d\n", (LOG))

Likewise.

> +#define err_assert(_exp)                                        \
> +        if (!(_exp))                                            \
> +          {                                                     \
> +            printf ("assert fail %s:%d\n",__FILE__,__LINE__);   \
> +            exit (1);                                           \
> +          }

Please just use gcc_assert() within gcc.  There were quite a few
instances where you'd used this macro instead.

> +#define REG_CONTAIN(REGNO,FIRST,NUM)    ((unsigned int)((int) (REGNO) - (FIRST)) < (NUM))
> +#define GP_REG_P(REGNO)                 REG_CONTAIN (REGNO, GP_REG_FIRST, GP_REG_NUM)

Long lines.

> +#define CE_REG_P(REGNO)                 REG_CONTAIN (REGNO, CE_REG_FIRST, CE_REG_NUM)

Long line.

> +#define UIMM_IN_RANGE(_v,_w)            ((_v) >= 0 && (_v) < ((long long)1 << (_w)))
>
> +#define SIMM_IN_RANGE(_v,_w)                            \
> +        ((_v) >= (-1 * ((long long)1 << ((_w)-1))) &&   \
> +         (_v) <  ( 1 * ((long long)1 << ((_w)-1))) )
>
> +#define IMM_IN_RANGE(_v,_w,_s)          ((_s) ? SIMM_IN_RANGE (_v,_w):UIMM_IN_RANGE(_v,_w))

Long lines and formatting.

GCC convention is to use upper-case names (without an underscore prefix)
in macro names.

Use "(HOST_WIDE_INT)" rather than "(long long)".

> +#define CONST_HIGH_PART(VALUE)          (((VALUE) + 0x8000) & ~(unsigned HOST_WIDE_INT) 0xffff)

Long line.

> +#define CONST_LOW_PART(VALUE)           ((VALUE) - CONST_HIGH_PART(VALUE))

Formatting.

> +enum score_symbol_type {
> +        SYMBOL_GENERAL,
> +        SYMBOL_SMALL_DATA
> +};

Likewise.

> +#define BITSET_P(VALUE, BIT)      (((VALUE) &(1L <<(BIT))) != 0)

Likewise.

> +  if ((mode = GET_MODE (op)) == VOIDmode)
> +    mode = DImode;

Please separate out the assignment.

> +  if (GET_CODE (op) == REG &&  REGNO (op) == HI_REGNUM )

Formatting.

> +struct score_frame_info*

Likewise.

> +struct score_frame_info*

Likewise.

> +      if (score_save_reg_p(regno))

Likewise.

> +      offset = (f->args_size + f->cprestore_size + f->var_size +
> +                f->gp_reg_size - GET_MODE_SIZE (SImode));

Likewise.

> +bool
> +mdx_prologue (void)
> +{

This function is missing a comment.  In particular, why is it returning
a bool that's always true?  As with movsicc, there's no sensible behaviour
if this function returns false.

There are other functions in this file without comments too.

> +#define EMIT_PL(_rtx)        RTX_FRAME_RELATED_P(_rtx) = 1

Likewise.

> +  struct score_frame_info* f = mda_compute_frame_size (get_frame_size ());

Likewise.

> +  size  = f->total_size - f->gp_reg_size;

Likewise.

> +  for (regno = (int)GP_REG_LAST; regno >= (int)GP_REG_FIRST; regno--)

Likewise.  (Although why are these casts needed?)

> +          EMIT_PL (emit_move_insn (gen_rtx_REG (Pmode,PROLOGUE_TEMP_REGNUM),

Formatting.

> +                                             gen_rtx_REG (Pmode,PROLOGUE_TEMP_REGNUM))));

Long line and formatting.

> +                                          gen_rtx_SET (VOIDmode, stack_pointer_rtx,

Long line.
> +                                                       plus_constant (stack_pointer_rtx, -size)),

Likewise.

> +bool
> +mdx_epilogue (int sibcall_p)
> +{

As for mdx_prologue.

> +  struct score_frame_info* f = mda_compute_frame_size (get_frame_size ());

Formatting.

> +  size  = f->total_size - f->gp_reg_size;

Likewise.

> +          emit_move_insn (gen_rtx_REG (Pmode,EPILOGUE_TEMP_REGNUM),

Likewise.

> +  for (regno = (int)GP_REG_FIRST; regno <= (int)GP_REG_LAST; regno++)

As above.

> +mdx_movsi (rtx* ops)

Formatting.

> +      && !register_operand (src, SImode) )

Likewise.

> +mdx_movdi (rtx* ops)

Likewise.

> +      && !register_operand (src, DImode) )

Likewise.

> +static rtx  cmp_op0, cmp_op1;

Excess space.

> +  switch ((info->code = GET_CODE (x)))

Please separate out the assignment.

> +      info->reg  = XEXP(x, 0);

Formatting.

> +      if (TARGET_NOPINDEX || GET_MODE_SIZE(mode) > GET_MODE_SIZE(SImode))

Likewise.

> +  err_assert (0);

Should be gcc_unreachable ().

> +bool
> +mdx_movsicc (rtx* ops)

Formatting.  This is another case where your function always returns true,
and where the behaviour for a future false return would be wrong.

> +  if (GET_CODE (ops[1]) == EQ || GET_CODE (ops[1]) == NE )

Formatting.

> +  emit_insn (gen_movsicc_internal (ops[0],ops[1],ops[2],ops[3]));

Likewise.

> +bool
> +mdx_cmpsi (rtx* ops)

As for mdx_movsicc.

> +bool
> +mdx_call (rtx* ops, bool sib)

As for mdx_movsicc.

> +  rtx addr  = XEXP (ops[0], 0);

Formatting.

> +bool
> +mdx_call_value (rtx* ops, bool sib)

As for mdx_movsicc.

> +  rtx result= ops[0];

Formatting.

> +    ldst = subw (dst, 0);
> +
> +  if (GET_CODE (ldst) == REG && reg_overlap_mentioned_p (ldst, src))
> +    {
> +      emit_move_insn (subw (dst, 1), subw (src, 1));
> +      emit_move_insn (subw (dst, 0), subw (src, 0));
> +    } else {

Likewise.

> +      emit_move_insn (subw (dst, 0), subw (src, 0));
> +      emit_move_insn (subw (dst, 1), subw (src, 1));
> +    }

It seems a little inefficient to generate subw (dst, 0) again.
ldst is just thrown away.

> +mds_zero_extract_andi(rtx* ops)

Formatting.

> +  else {
> +    unsigned HOST_WIDE_INT mask;
> +    mask = (0xffffffffU & ((1U << INTVAL(ops[1])) - 1U));
> +    mask = mask << INTVAL(ops[2]);
> +    emit_insn(gen_andsi3_cmp(ops[0],GEN_INT(mask)));
> +  }

Likewise (the whole block has problems).  Again, you need to use
gen_int_mode (..., SImode) here.

> +  emit_insn (gen_andsi3_cmp ( XEXP (ops[0], 0), GEN_INT (mask)));

Formatting.

> +  if (GET_CODE(addr) == MEM)

Formatting.

> +pr_addr_post (rtx* ops, int idata, int iaddr,
> +              char* ip, enum mda_mem_unit unit)

Formatting.

> +  if (!mda_pindex_mem(ops[iaddr])

Formatting.

> +          return snprintf (ip, INS_BUF_SZ, "!        %%%d, [%%%d]",idata, iaddr);

Long line.

> +              return snprintf (ip, INS_BUF_SZ, "p!        %%%d, %%c%d", idata, iaddr);

Likewise.

> +const char*
> +mdp_linsn (rtx* ops, enum mda_mem_unit unit, bool sign)

Formatting.

> +  const char* pre_ins[] ={"lbu", "lhu","lw", "??",
> +                          "lb" , "lh" ,"lw", "??"};
> +  char* ip;

Likewise.

> +  strcpy (ins, pre_ins[(sign ? 4 : 0) + unit]);
> +  ip = ins + strlen (ins);
> +
> +

Excess lines.

> +      || ( sign && unit != MDA_BYTE ))

Formatting.

> +const char*
> +mdp_sinsn (rtx* ops, enum mda_mem_unit unit)

Likewise.

> +  const char* pre_ins[] = {"sb" , "sh" , "sw"};
> +
> +  char*ip;

Likewise.  Blank line after the last line rather than before?

> +const char*
> +mdp_limm (rtx* ops)

Formatting.

> +  else if (CONST_OK_FOR_LETTER_P (INTVAL (ops[1]), 'Q'))

Following on from above, this should be:

  else if (EXTRA_CONSTRAINT (operands[1], 'Q'))

> +const char*
> +mdp_move (rtx* ops)

Formatting.

> +    }else if (G16_REG_P (REGNO (ops[1])))
> +      {
> +        return "mhfl!   %0, %1";
> +      }else
> +        return "mv      %0, %1";

Likewise.

> +  if ((INTVAL (len) % BITS_PER_WORD) || (INTVAL (off) % BITS_PER_UNIT) )

Likewise.

> +  if ((INTVAL (len) % BITS_PER_WORD) || (INTVAL (off) % BITS_PER_UNIT) )

Likewise.

> +  if (GET_CODE (len) != CONST_INT || INTVAL (len) > (14 * UNITS_PER_WORD) )
> +    return false;

Please don't hard-code 14 here.  At least use a macro.

> +  sz = TARGET_LIT_ENDIAN ? INTVAL (ali): 4;

Formatting.

> +                      MIN(MEM_ALIGN(src), MEM_ALIGN(dst)), 0);

Likewise.

> +  mode= mode_for_size (sz * BITS_PER_UNIT, MODE_INT, 0);

Likewise.

> +          emit_move_insn (regs[n], adjust_address (src,mode,n*sz));

Likewise.

> +          emit_move_insn (adjust_address (dst,mode,n*sz), regs[n]);

Likewise.

> +          emit_move_insn (sp, XEXP(src, 0));

Likewise.

> +        /* Mop up any left-over bytes.  */
> +    if (INTVAL (len) > (n * sz))

Odd identation.

> +const char*
> +mdp_add_imm_ucc (rtx* ops)

Formatting.

> +  err_assert (GET_CODE (ops[2])== CONST_INT);

Likewise.

> +      if (v > 0 && num_bits1(v) == 1 && IMM_IN_RANGE (ffs (v) - 1, 4, 0))

Likewise.

> +      if (v < 0 && num_bits1 (-v) == 1 && IMM_IN_RANGE (ffs (-v)-1, 4, 0))

Likewise.

> +const char*
> +mdp_select (rtx* ops, const char* inst_pre, bool commu, const char* let)

Likewise.

> +  switch (which_alternative)
> +    {
> +        case 0: return mdp_limm (operands);
> +        case 1: return mdp_move (operands);
> +        case 2: return mdp_linsn (operands, MDA_BYTE, false);
> +        case 3: return mdp_sinsn (operands, MDA_BYTE);
> +        case 4: return TARGET_MAC? \"mf%1%S0 %0\": \"mf%1    %0\";
> +        case 5: return TARGET_MAC? \"mt%0%S1 %1\": \"mt%0    %1\";
> +        case 6: return \"mfsr    %0, %1\";
> +        case 7: return \"mtsr    %1, %0\";
> +        default: abort ();
> +    }

Here and elsewhere, you do not need backslashes before the double quotes.
All "abort ();"s in gcc code should be "gcc_unreachable ();"s.

All your switch statements in this file have formatting problems.

> +  switch (which_alternative)
> +    {
> +        case 0:
> +                if( GET_CODE (operands[1]) == SYMBOL_REF ||
> +                        GET_CODE (operands[1]) == LABEL_REF)
> +                        return \"la      %0, %1\";
> +                else if (GET_CODE (operands[1]) == CONST &&
> +                         GET_CODE (XEXP (operands[1], 0)) == PLUS &&
> +                         GET_CODE (XEXP (XEXP (operands[1], 0), 0)) == SYMBOL_REF)
> +                        return \"la      %0, %1\";
> +                else
> +                        return mdp_limm (operands);
> +        case 1: return mdp_move (operands);
> +        case 2: return mdp_linsn (operands, MDA_WORD, false);
> +        case 3: return mdp_sinsn (operands, MDA_WORD);
> +        case 4: return TARGET_MAC? \"mf%1%S0 %0\": \"mf%1    %0\";
> +        case 5: return TARGET_MAC? \"mt%0%S1 %1\": \"mt%0    %1\";
> +        case 6: return \"mfsr    %0, %1\";
> +        case 7: return \"mtsr    %1, %0\";
> +        case 8: return \"mfcr    %0, %1\";
> +        case 9: return \"mtcr    %1, %0\";
> +        default: abort ();
> +    }

Lots of formatting nits ;)  What is the purpose of all the conditionals
in alternative 0?  They seem overly complicated.  It might be cleaner
and more robust to make mdp_limm handle symbolic constants too,
with something like:

  if (GET_CODE (ops[1]) != CONST_INT)
    return "la      %0, %1";

> +(define_insn "addsi3"
> +  [(set (match_operand:SI 0 "register_operand" "=d,d,d")
> +        (plus:SI (match_operand:SI 1 "register_operand"  " 0,d,d")
> +                 (match_operand:SI 2 "arith_operand"     " L,N,d")))]
> +  ""
> +  "@
> +   addi    %0, %c2
> +   addri   %0, %1, %c2
> +   add     %0, %1, %2"
> +  [(set_attr "type" "arith")
> +   (set_attr "mode" "SI")])

You can get better code by adding "%" to beginning of operand 1's
constraints.  This tells constraint-based code that operands 1 and 2
are commutative.  There are a few other examples later in the file.

> +(define_insn "*subsi3_cmp"
> +  [(set (reg:CC_NZ CC_REGNUM)
> +        (compare:CC_NZ  (minus:SI (match_operand:SI 0 "register_operand" " d")

Excess space after CC_NZ.  Several other occurences later.

> +(define_peephole2
> +  [(set (match_operand:SI 0 "register_operand" "")
> +        (minus:SI (match_operand:SI 1 "register_operand" "")
> +                  (match_operand:SI 2 "register_operand" "")))
> +   (set (reg:CC CC_REGNUM)
> +        (compare:CC (match_dup 1) (match_dup 2)))]
> +  "GP_REG_P (REGNO (operands[0])) &&
> +   GP_REG_P (REGNO (operands[1])) &&
> +   GP_REG_P (REGNO (operands[2]))"

Formatting of "&&"s.  Why not use your g32reg_operand predicate instead?

> +(define_insn "iorsi3_ucc"
> +  [(set (reg:CC_NZ CC_REGNUM)
> +        (compare:CC_NZ  (ior:SI (match_operand:SI 1 "register_operand" " d")
> +                                (match_operand:SI 2 "arith_operand"    " d"))

Another arith_operand that only accepts registers.  Several
other occurences later.

> +        (compare:CC_N   (ashiftrt:SI (ashift:SI (match_operand:SI 0 "register_operand"  " d")

Long line.

> +        (compare:CC_N   (ashiftrt:SI (ashift:SI (match_operand:SI 1 "register_operand" " d")

Likewise.

> +{ mda_gen_cmp (CCmode); })

Formatting.  (Same for all other branch patterns.)

> +(define_insn "branch_n"
> +  [(set (pc)
> +        (if_then_else
> +         (match_operator 0 "comparison_operator"
> +                         [(reg:CC_N CC_REGNUM)
> +                          (const_int 0)])
> +         (label_ref (match_operand 1 "" ""))
> +         (pc)))]
> +  "GET_CODE (operands[0]) == LT ||
> +   GET_CODE (operands[0]) == GE "

Formatting.  It might be cleaner to use a tighter predicate than
comparison_operator instead of checking GET_CODE like this.

> +  "b%C0    %1"
> +  [(set_attr "type" "branch")])
> +
> +(define_insn "branch_nz"
> +  [(set (pc)
> +        (if_then_else
> +         (match_operator 0 "comparison_operator"
> +                         [(reg:CC_NZ CC_REGNUM)
> +                          (const_int 0)])
> +         (label_ref (match_operand 1 "" ""))
> +         (pc)))]
> +  "GET_CODE (operands[0]) == EQ ||
> +   GET_CODE (operands[0]) == NE ||
> +   GET_CODE (operands[0]) == LT ||
> +   GET_CODE (operands[0]) == GE "

Likewise.

> +  "b%C0    %1"
> +  [(set_attr "type" "branch")])
> +
> +(define_insn "branch_cc"
> +  [(set (pc)
> +        (if_then_else
> +         (match_operator 0 "comparison_operator"
> +                         [(reg:CC CC_REGNUM)
> +                          (const_int 0)])
> +         (label_ref (match_operand 1 "" ""))
> +         (pc)))]
> +  ""
> +  "b%C0    %1"
> +  [(set_attr "type" "branch")])

It's usual to define inverted versions of each branch as well,
in which the (label_ref ...) comes after (pc).  It used to lead
to better code, although I'm not sure how much of a difference
it makes these days.

> +  if (GET_CODE (dest) != REG ||
> +      GET_MODE (dest) != Pmode)

Formatting.

> +CC_MODE(CCFP);
> +CC_MODE(CCFPE);
> +CC_MODE(CC_N);
> +CC_MODE(CC_NZ);

Some comments would be welcome here, saying what each mode is used for.
CCFP and CCFPE appear to be unused.

> Index: gcc/config/score/score-mdaux.h
> ===================================================================
> --- gcc/config/score/score-mdaux.h	(revision 0)
> +++ gcc/config/score/score-mdaux.h	(revision 0)

Lots of formatting issues here, but I think you've got the idea by now ;)

Richard


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