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