Re: [PATCH][4.6][ARM] New CPU support for Faraday cores

["M.F. Wu" <mingfeng6865 at gmail dot com>]

Dear Ramana,

Thank you for your advisement.

                                  Mingfeng Wu

2010/11/24 Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>:
> Hi Mingfeng,
>
> I'm not a maintainer and cannot approve or reject your patch. However I
> have a few comments / queries regarding your patch.
>
>
>>
>> diff -uNr tmp/gcc-4.6-svn-20101116/gcc/ChangeLog
>> gcc-4.6-svn-20101116/gcc/ChangeLog
>> --- tmp/gcc-4.6-svn-20101116/gcc/ChangeLog ? ?2010-11-23
>> 13:43:41.725061000 +0800
>> +++ gcc-4.6-svn-20101116/gcc/ChangeLog ? ? ? ?2010-11-23
>> 14:40:20.796047000 +0800
>> @@ -1,3 +1,24 @@
>> +2010-11-22 ?Toolchain ?<toolchain@faraday-tech.com>
>>
> It's generally been the practice to have a Changelog entry refer to real
> people rather than a generic support address. I believe Faraday Tech has
> a generic copyright assignment on file that covers this contribution but
> it's usually been our practice to state explicit authors. Thus I think
> that the authors of this patch should be named in the Changelog entry
> even if there are multiple authors.
>

I have added the authors to the list and modified the Changelog entry.

>>
>> +
>> + ? ? Add Faraday CPU support -
>> FA526/FA626/FA606TE/FA626TE/FMP626/FA726TE.
>> + ? ? Modify files:
>> + ? ? * config/arm/arm-cores.def
>> + ? ? * config/arm/arm-tune.md
>> + ? ? * config/arm/arm.c
>> + ? ? * config/arm/arm.md
>> + ? ? * config/arm/bpabi.h
>> + ? ? * config/arm/t-arm
>> + ? ? * config/arm/t-arm-elf
>> + ? ? * config/arm/t-linux-eabi
>> + ? ? * doc/invoke.texi
>> + ? ? New added files:
>> + ? ? * config/arm/fa526.md
>> + ? ? * config/arm/fa626.md
>> + ? ? * config/arm/fa606te.md
>> + ? ? * config/arm/fa626te.md
>> + ? ? * config/arm/fmp626.md
>> + ? ? * config/arm/fa726te.md
>> +
>>
>
> Please create a ChangeLog as specified in the GNU coding standards
> linked from http://gcc.gnu.org/contribute.html#standards . The ChangeLog
> should not be a part of the final patch you submit but a part of the
> mail you send out.
>
> Please look at the Changelog file or other patches in the mailing list
> archives for other examples in that area. Thanks.
>
> If there are new files (like fa526.md etc.) they can just be marked as
> New. ?If there are changes to existing files for e.g. config/arm/arm.c ,
> a small description of the changes to them must be listed in the
> Changelog entry.
>
>
>
>> > diff -uNr tmp/gcc-4.6-svn-20101116/gcc/config/arm/arm.c
>> gcc-4.6-svn-20101116/gcc/config/arm/arm.c
>> > --- tmp/gcc-4.6-svn-20101116/gcc/config/arm/arm.c ? ? 2010-11-23
>> 13:43:46.895582000 +0800
>> > +++ gcc-4.6-svn-20101116/gcc/config/arm/arm.c 2010-11-23
>> 13:57:43.649325000 +0800
>> > @@ -128,6 +128,7 @@
>> > ?static void thumb1_output_function_prologue (FILE *,
>> HOST_WIDE_INT);
>> > ?static int arm_comp_type_attributes (const_tree, const_tree);
>> > ?static void arm_set_default_type_attributes (tree);
>> > +static int arm_sched_variable_issue (FILE *, int, rtx, int);
>> > ?static int arm_adjust_cost (rtx, rtx, rtx, int);
>> > ?static int count_insns_for_constant (HOST_WIDE_INT, int);
>> > ?static int arm_get_strip_length (int);
>> > @@ -239,6 +240,7 @@
>> > ?static rtx arm_pic_static_addr (rtx orig, rtx reg);
>> > ?static bool cortex_a9_sched_adjust_cost (rtx, rtx, rtx, int *);
>> > ?static bool xscale_sched_adjust_cost (rtx, rtx, rtx, int *);
>> > +static bool fa726te_sched_adjust_cost (rtx, rtx, rtx, int *);
>> > ?static enum machine_mode arm_preferred_simd_mode (enum
>> machine_mode);
>> > ?static bool arm_class_likely_spilled_p (reg_class_t);
>> > ?static bool arm_vector_alignment_reachable (const_tree type, bool
>> is_packed);
>> > @@ -350,6 +352,9 @@
>> > ?#undef ?TARGET_SET_DEFAULT_TYPE_ATTRIBUTES
>> > ?#define TARGET_SET_DEFAULT_TYPE_ATTRIBUTES
>> arm_set_default_type_attributes
>> >
>> > +#undef ?TARGET_SCHED_VARIABLE_ISSUE
>> > +#define TARGET_SCHED_VARIABLE_ISSUE arm_sched_variable_issue
>> > +
>> > @@ -7913,6 +7925,56 @@
>> > ? ?return true;
>> > ?}
>> >
>> > +/* Adjust cost hook for FA726TE. ?*/
>> > +static bool
>> > +fa726te_sched_adjust_cost (rtx insn, rtx link, rtx dep, int * cost)
>> > +{
>> > + ?/* For FA726TE, true dependency on CPSR (i.e. set cond followed
>> by predicated)
>> > + ? ? have penalty of 3 */
>> > + ?if (REG_NOTE_KIND (link) == REG_DEP_TRUE
>> > + ? ? ?&& recog_memoized (insn) >= 0
>> > + ? ? ?&& recog_memoized (dep) ?>= 0
>> > + ? ? ?&& get_attr_conds (dep) == CONDS_SET)
>> > + ? ?{
>> > + ? ? ?/* Use of carry (e.g. 64-bit arithmetic) in ALU: 3-cycle
>> latency */
>> > + ? ? ?if (get_attr_conds(insn) ?== CONDS_USE &&
>> > + ? ? ? ? ?get_attr_type(insn) != TYPE_BRANCH)
>> > + ? ? ? ?{
>> > + ? ? ? ? ?*cost = 3;
>> > + ? ? ? ? ?return false;
>> > + ? ? ? ?}
>> > +
>> > + ? ? ?if (GET_CODE (PATTERN (insn)) == COND_EXEC
>> > + ? ? ? ? ?|| get_attr_conds(insn) ?== CONDS_USE)
>> > + ? ? ? ?{
>> > + ? ? ? ? ?*cost = 0;
>> > + ? ? ? ? ?return false;
>> > + ? ? ? ?}
>> > + ? ?}
>> > +
>> > + ?return true;
>> > +}
>> > +
>> > +/* Determine how many instructions can we issue. Fixup the issue
>> that some
>> > + ? UNSPECs get scheduled. */
>> > +static int
>> > +arm_sched_variable_issue (FILE *f ATTRIBUTE_UNUSED,
>> > + ? ? ? ? ? ? ? ? ? ? ? ? ? int verbose ?ATTRIBUTE_UNUSED, rtx insn,
>> int more)
>
> Can you make sure that int verbose is aligned to be just below the
> column for FILE *f ?
>
>>
>> > +{
>> > + ?if (arm_tune == fa726te
>> > + ? ? ?&& recog_memoized (insn) >= 0 /* insn recognizable? */
>> > + ? ? ?&& (get_attr_type (insn) == TYPE_ALU
>> > + ? ? ? ? ?|| get_attr_type (insn) == TYPE_ALU_SHIFT
>> > + ? ? ? ? ?|| get_attr_type (insn) == TYPE_ALU_SHIFT_REG))
>>
> Given that we are moving towards a backend where core specific
> information is
> stored in the tune_params structure , it might be an idea to add
> sched_variable_issue
> to the costs infrastructure initialize this to NULL in other cases
> and return more - 1 by default.
>

Function, arm_sched_variable_issue, has been removed.

>
>>
>> > + ?else
>> > + ? ?{
>> > + ? ? ? return more-1;
>>
> This should be "return more - 1;" Notice the extra spaces between more
> '-' and '1'.
>
>
>>
>> > diff -uNr tmp/gcc-4.6-svn-20101116/gcc/config/arm/fa526.md
>> gcc-4.6-svn-20101116/gcc/config/arm/fa526.md
>> > --- tmp/gcc-4.6-svn-20101116/gcc/config/arm/fa526.md ?1970-01-01
>> 08:00:00.000000000 +0800
>> > +++ gcc-4.6-svn-20101116/gcc/config/arm/fa526.md ? ? ?2010-11-23
>> 14:36:17.916371000 +0800
>>
>> <....>
>> snipped
>>
>> > +
>> >
>> +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
>> > +;; Branch and Call Instructions
>> >
>> +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
>> > +
>> > +;; Branch instructions are difficult to model accurately. ?The ARM
> s/ARM/FA526.
>
> And equivalently in all the other pipeline descriptions.
>

Sorry, I don't understand exactly what you mean...

>
>> >
>> > diff -uNr tmp/gcc-4.6-svn-20101116/gcc/config/arm/t-arm
>> gcc-4.6-svn-20101116/gcc/config/arm/t-arm
>> > --- tmp/gcc-4.6-svn-20101116/gcc/config/arm/t-arm ? ? 2010-11-23
>> 13:43:47.213582000 +0800
>> > +++ gcc-4.6-svn-20101116/gcc/config/arm/t-arm 2010-11-23
>> 13:57:43.738329000 +0800
>> > @@ -24,6 +24,11 @@
>> > ? ? ? ? ? ? ? $(srcdir)/config/arm/arm1020e.md \
>> > ? ? ? ? ? ? ? $(srcdir)/config/arm/arm1026ejs.md \
>> > ? ? ? ? ? ? ? $(srcdir)/config/arm/arm1136jfs.md \
>> > + ? ? ? ? ? ? $(srcdir)/config/arm/fa526.md \
>> > + ? ? ? ? ? ? $(srcdir)/config/arm/fa606te.md \
>> > + ? ? ? ? ? ? $(srcdir)/config/arm/fa626te.md \
>> > + ? ? ? ? ? ? $(srcdir)/config/arm/fmp626.md \
>> > + ? ? ? ? ? ? $(srcdir)/config/arm/fa726te.md \
>> > ? ? ? ? ? ? ? $(srcdir)/config/arm/arm926ejs.md \
>> > ? ? ? ? ? ? ? $(srcdir)/config/arm/cirrus.md \
>> > ? ? ? ? ? ? ? $(srcdir)/config/arm/fpa.md \
>> > diff -uNr tmp/gcc-4.6-svn-20101116/gcc/config/arm/t-arm-elf
>> gcc-4.6-svn-20101116/gcc/config/arm/t-arm-elf
>> > --- tmp/gcc-4.6-svn-20101116/gcc/config/arm/t-arm-elf 2010-11-23
>> 13:43:47.221580000 +0800
>> > +++ gcc-4.6-svn-20101116/gcc/config/arm/t-arm-elf ? ? 2010-11-23
>> 13:57:43.744348000 +0800
>> > @@ -36,6 +36,10 @@
>> > ?MULTILIB_EXCEPTIONS ?=
>> > ?MULTILIB_MATCHES ? ? =
>> > +#MULTILIB_OPTIONS ? ? +=
>> mcpu=fa526/mcpu=fa626/mcpu=fa606te/mcpu=fa626te/mcpu=fmp626/mcpu=fa726te/mcpu=arm926ej-s
>> > +#MULTILIB_DIRNAMES ? ?+= fa526 fa626 fa606te fa626te fmp626 fa726te
>> arm926ej-s
>>
>
> Even though these are commented I assume that these are in here because
> they are known
> to work and you expect it to keep working.
>
> Why do you have arm926ej-s here ?
>

Removed arm926ej-s.

>>
>> > +#MULTILIB_EXCEPTIONS ?+= *mthumb*/*mcpu=fa526 *mthumb*/*mcpu=fa626
>> > +
>>
>>
>> > ?#MULTILIB_OPTIONS ? ? ?+= march=armv7
>> > ?#MULTILIB_DIRNAMES ? ? += thumb2
>> > ?#MULTILIB_EXCEPTIONS ? += march=armv7* marm/*march=armv7*
>> > @@ -52,6 +56,8 @@
>> > ?MULTILIB_OPTIONS ? ? ? += mfloat-abi=hard
>> > ?MULTILIB_DIRNAMES ? ? ?+= fpu
>> > ?MULTILIB_EXCEPTIONS ? ?+= *mthumb/*mfloat-abi=hard*
>> > +MULTILIB_EXCEPTIONS ? ?+= *mcpu=fa526/*mfloat-abi=hard*
>> > +MULTILIB_EXCEPTIONS ? ?+= *mcpu=fa626/*mfloat-abi=hard*
>> >
>> > ?# MULTILIB_OPTIONS ? ?+= mcpu=ep9312
>> > ?# MULTILIB_DIRNAMES ? += ep9312
>> > diff -uNr tmp/gcc-4.6-svn-20101116/gcc/config/arm/t-linux-eabi
>> gcc-4.6-svn-20101116/gcc/config/arm/t-linux-eabi
>> > ---
>> tmp/gcc-4.6-svn-20101116/gcc/config/arm/t-linux-eabi ? ? ?2010-11-23
>> 13:43:47.231588000 +0800
>> > +++ gcc-4.6-svn-20101116/gcc/config/arm/t-linux-eabi ?2010-11-23
>> 13:57:43.750343000 +0800
>> > @@ -24,6 +24,10 @@
>> > ?MULTILIB_OPTIONS ? ? =
>> > ?MULTILIB_DIRNAMES ? ?=
>> > +#MULTILIB_OPTIONS ? ? +=
>> mcpu=fa606te/mcpu=fa626te/mcpu=fmp626/mcpu=fa726te
>> > +#MULTILIB_DIRNAMES ? ?+= fa606te fa626te fmp626 fa726te arm926ej-s
>>
> These variables even though commented out should work correctly when
> uncommented.
>
> Why does this have arm926ej-s for MULTILIB_DIRNAMES with no
> corresponding entry for mcpu=arm926ej-s in MULTILIB_OPTIONS ? Each
> entry in MULTILIB_DIRNAMES must have an equivalent entry in
> MULTILIB_OPTIONS. Even though the extra entry for arm926ej-s is
> superfluous, it would be better to remove it from here.
>

Removed arm926ej-s.

>
> Cheers
> Ramana
>
>
>

Attachment: gcc-4.6-svn-20101116-faraday-cpu.patch
Description: Binary data