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: [PATCH][4.6][ARM] New CPU support for Faraday cores


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.

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


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


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

> 
> > +#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. 


Cheers
Ramana



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