This is the mail archive of the 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: New: CR16 Port (Take 3)

Hello All,
A humble reminder to review the patch and suggest if this is OK for trunk.

Pompapathi V Gadad wrote:
Hello Michael,
Sorry for delay in submitting the modified patch. I have incorporated all suggestions
that I have mentioned in the previous mail (See attached). Only one exception is that
I did not know where to document that this port supports only 32-bit floating point.
It would be helpful, if you can point the right place.
Please find the new patch attached: cr16-port_gcc_16oct07.diff
I have run the "compile-only" gcc testsuite for the new patch. There are no additional
failures due to modification.

Please review the patch and suggest if this is OK for trunk.

Thanks in advance,

Pompapathi V Gadad wrote:
Hello Michael,
Thanks for reviewing in such detail. Please see my answers below. I will
incorporate many of your suggestions immediately.

About your general comments:
>1) Your formatting of the lisp code in the is inconsistant. The
> simplest approach is to go in with gnu emacs, and hit tab at the beginning
> of the line to let emacs reformat it. For example, instead of:
I will make necessary modification.

>3) Some places, particularly in the .md file you don't have the space before
> the '(' in calls. For example instead of:
I will make necessary modification.
>4) You call abort () in several places. Those should be changed to
> gcc_unreachable ().
I guess, this is very important. I will do it right away.

>5) There are several places that when you split up a long line, you have the
> operator at the end of the line instead of at the beginning of the next
> line. For example instead of:
I will make necessary modification.
About your specific comments:
>1) I am somewhat wary of you mov<mode> code. You probably need to define the
> appropriate reload_{out,in}<mode> operations to properly move stuff between
> registers, and do the reloading.
Can you please explain what was the observation. This could be a major change to the port.
Therefore I would want to recode it is carefully, if necessary.

>2) Technically fixing the size of double to 32-bits violates ANSI/ISO C, but
> having supported 16-bit embedded platforms in the past, floating point is
> often just not done in that environment. You might get several test
> failures that want to use 64-bit floating point. I see that you don't
> support trampolines either, you might document this restrictions in the
> texinfo file on CR16 (and any other restrictions that you have).
Well, I can't help. CR16 has no support for 64-bit arithmetic and I guess, it is
not intended as well. I will document the unsupported features/restrictions as well.

> should use HOST_WIDE_INT instead of long long.
Important point. I will do it right away.

>4) You defined REG_CLASS_FROM_LETTER. At some point in the future, you
> probably should migrate this to using define_register_constraint in the .md
> file.
May be it is right time to migrate. I will plan for this.

>5) On the insv define_expand, you probably should define 2 new predicates and
> eliminate the tests inside of the {}. One predicate would only match a
> const_int 1, and the other would match 0 or 1. So instead of:
Your suggestion is much better. I will incorporate and test it.

>6) cr16_prepare_push_pop_string allocates memory which is never freed up.
> Either use a static buffer, or use asm_printf and print out the string
> directly.
Yes. This is an issue. I will incorporate your suggestion.

>7) The HI division is generic, this might be useful to move into libgcc2 for
> other 16-bit ports (I dunno whether there are more 16 bit ports).
I did not want to change the files that would affect other ports.
May be in future I will submit another patch.

>8) In ashr<mode> and lshr<mode>, you use copy_to_mode_reg, when instead you
> should use predicates to restrict the type to the appropriate types:
I will incorporate your suggestion.

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