Re: How can I add S+core backend code into GCC package

[Richard Sandiford <richard at codesourcery dot com>]

liqin@sunnorth.com.cn writes:
> Thanks your patient reply, we spent a few days to update our code for 
> Score backend.
>
> 1. We had updated all the formatting problems and most other problems you 
> pointed out.
> 2. Some code we think it is reasonable for our condition, so we not do any 
> change.
> 3. And some code we are not sure how to make change(how to make it more 
> suitable for reading and understanding), we had cut them down.
> 4. The comments in score.h(some we copy from gcc internal) had made some 
> change, but others we think it is more clear to describe the macro than 
> ours, so we are not updating these comments.
>
> We had carefully read your reply and updated our code, but we know you 
> still could find some problems, so we are looking forward to get your 
> reply about our patch. we will continue to update the code until it attain 
> the gcc code standard.

Thanks, you've obviously put a lot of effort into this update.  FWIW,
the new patch looks good to me.  I've mentioned one or two very minor
niggles below, but they're so trivial that I don't think they need
a full retest.  I probably wouldn't have mentioned them at all if
it wasn't for the CCmodes thing, and the new use of C++-style comments.

As far as point (3) in your message above goes, I notice you've removed
the unaligned load, unaligned store and block move patterns.  I can
fully understand why you've done that, especially so close to the next
release.  It is something that gcc can do, but it can be tricky to get
right.

> +int reg_class_mask[][5] = REG_CLASS_CONTENTS;

This variable is no longer used after the TEST_HARD_REG_BIT change.

> +/* Implement CONST_OK_FOR_LETTER_P macro.  */
> +/* imm constraints
[...]
> +   Q        const_hi    imm  */
> +int
> +score_const_ok_for_letter_p (int value, char c)

'Q' doesn't really belong in this comment.  I think you should instead
document both 'Q' and 'Z' above:

> +/* Implement EXTRA_CONSTRAINT macro.  */
> +int
> +score_extra_constraint (rtx op, char c)

...this function.  I notice you've removed the 'W' constraint, and as
far as I can tell, it was indeed unused.  Have I got that right?

> +      gcc_assert (0);

Should be gcc_unreachable ();

> +/* Implement SELECT_CC_MODE macro.  */
> +enum machine_mode
> +score_select_cc_mode (enum rtx_code op, rtx x, rtx y)
> +{
> +  if (GET_MODE (x) == SImode && y == const0_rtx)
> +    {
> +      switch (GET_CODE (x))
> +        {
> +        case PLUS:
> +        case MINUS:
> +        case AND:
> +        case ZERO_EXTRACT:
> +        case ZERO_EXTEND:
> +        case IOR:
> +        case XOR:
> +        case NOT:
> +        case LSHIFTRT:
> +        case ASHIFT:
> +          if (op == EQ || op == NE || op == LT || op == GE)
> +            return CC_NZmode;
> +          break;
> +        case ASHIFTRT:
> +          if (op == EQ || op == NE)
> +            return CC_NZmode;
> +          if (op == LT || op == GE)
> +            return CC_Nmode;
> +        case SIGN_EXTEND:
> +          if (op == LT || op == GE)
> +            return CC_Nmode;
> +          break;
> +        default:
> +          break;
> +        }
> +    }
> +
> +  return CCmode;
> +}

You've added this function in the new patch, but commented out its use.
Is there a problem with it?  Perhaps you included it to show why
CC_NZmode and CC_Nmode is needed, and where the mode is supposed to be
introduced.  However, it would be nice to have a comment saying why the
function isn't used at present.

> +    but Control register have 32 reisters, cr16-cr31.  */

typo: "reisters".  (I only mention it because I noticed it ;)  The gcc
sources are probably rife with such things, despite Kazu's efforts.)

> +/* Condition Code Status.  */
> +/* Given a comparison code (EQ, NE, etc.) and the first operand of a COMPARE,
> +   return the mode to be used for the comparison.  */
> +//#define SELECT_CC_MODE(OP, X, Y)        score_select_cc_mode (OP, X, Y)
> +
> +/* Return nonzero if MODE implies a floating point inequality can be
> +   reversed.  */
> +//#define REVERSIBLE_CC_MODE(MODE)        1

As above, these lines are new to this version of the patch.  I don't
quite understand why they're disabled.  If you do continue to disable
them, you shouldn't use C++ style comments.  Either use /* */ or #if
0/#endif.  A comment would be nice too.  (This is the sort of area where
people might make backend interface changes, and might need to change
the S+core backend to the new interface.  They'd then need to understand
why the code is disabled.)

> +  SYMBOL_SMALL_DATA  // The symbol refers to something in a small data section.

Another new C++ style comment here.

> +/* Generate the prologue instructions for entry into a S+core function.  */
> +void
> +mdx_prologue (void)
> +{
> +#define EMIT_PL(_rtx)        RTX_FRAME_RELATED_P (_rtx) = 1
[...]
> +  return;
> +#undef EMIT_PL
> +}

No need for this final "return;".  Same goes the mdx_epilogue,
mdx_movsicc, mdx_call and mdx_call_value.

> +//bool mdx_block_move (rtx *ops);
> +//bool mdx_unaligned_load (rtx *ops);
> +//bool mdx_unaligned_store (rtx *ops);

C++-style comments.  It might be better to just remove these prototypes
and add them back when you add the definitions back.

Richard