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] |
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
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |