[PATCH][4.6][ARM] New CPU support for Faraday cores
M.F. Wu
mingfeng6865@gmail.com
Thu Dec 2 08:27:00 GMT 2010
Dear Ramana,
Thank you for your comments about the patch.
The patch has been modified as the attached
file shows.
The Changlog:
2010-12-02 Sanjin Liu <scliu@faraday-tech.com>
Mingfeng Wu <mingfeng@faraday-tech.com>
* config/arm/arm-cores.def: Add Faraday CPU support -
fa526/fa626/fa606te/fa626te/fmp626/fa726te.
* config/arm/arm-tune.md: Regenerate.
* config/arm/arm.c (arm_fa726te_tune): New tune_params for fa726te.
(fa726te_sched_adjust_cost): New cost function for fa726te.
(arm_issue_rate): Add fa726te.
* config/arm/arm.md (generic_sched): Add Faraday cores to generic_sched
and include machine description files.
* config/arm/bpabi.h (TARGET_FIX_V4BX_SPEC): Add fa526 and fa626.
* config/arm/t-arm (MD_INCLUDES): Include machine description files for
Faraday cores.
* config/arm/t-arm-elf: Add multilib option for Faraday cores.
* config/arm/t-linux-eabi: Add multilib option for Faraday cores except
fa526 and fa626.
* doc/invoke.texi: Document -mcpu for Faraday cores.
* config/arm/fa526.md: New file.
* config/arm/fa626.md: New file.
* config/arm/fa606te.md: New file.
* config/arm/fa626te.md: New file.
* config/arm/fmp626.md: New file.
* config/arm/fa726te.md: New file.
2010/11/30 Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>:
> Hi Mingfeng,
>
> Thanks for making these changes.
>
> Please do not make the Changelog a part of the final patch. Please make
> this a part of your final mail submission and not a part of your patch.
>
>
>> @@ -7913,6 +7921,36 @@
>
> <...>
>
>> + /* Use of carry (e.g. 64-bit arithmetic) in ALU: 3-cycle latency */
>
> Full stop at the end of comment followed by 2 spaces before end of comment.
> Can you please audit your patch to check for these issues ?
>
> This is true for comments in the machine description parts of your patch as well.
>
Fixed.
>
>> + if (get_attr_conds(insn) == CONDS_USE &&
>> + get_attr_type(insn) != TYPE_BRANCH)
>> + {
>> + *cost = 3;
>> + return false;
>> + }
>
> Space between function name and paranthesis. Thus it should be
> get_attr_conds (insn) and not get_attr_conds(insn) as above.
>
>> +
>> + if (GET_CODE (PATTERN (insn)) == COND_EXEC
>> + || get_attr_conds(insn) == CONDS_USE)
>> + {
>> + *cost = 0;
>> + return false;
>> + }
>> + }
>
> Likewise.
>
Fixed.
>
>> 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
>>
>> >> +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
>> >> > +;; Branch and Call Instructions
>> >> >
>> >> +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
>> >> > +
>
>> > And equivalently in all the other pipeline descriptions.
>> >
>>
>> Sorry, I don't understand exactly what you mean...
>
>
>> +;; Branch instructions are difficult to model accurately. The ARM
>> +;; core can predict most branches. If the branch is predicted
>>
> In the sentence above -
> Replace ARM with FA526 and similarly in the other pipeline descriptions.
>
> The FA526 core isn't one made by ARM and am not sure if you can use the name
> in that regard here :)
>
OK, you are right. I have modified the "ARM" to our core.
>> > ;
>> >
>> > +const struct tune_params arm_fa726te_tune =
>> > +{
>> > + arm_9e_rtx_costs,
>> > + fa726te_sched_adjust_cost,
>> > + 1
>> > +};
>> > +
>
> This part of your patch is now out-of-date thanks to Ian Bolton's latest commits in that area with respect
> to preloads. You might want to consider that in your final submission. I suppose using the defaults
> and turning off preloads at O3 would be the correct thing to do to get your patch sheperded through.
>
OK. I modified the arm_fa726te_tune.
const struct tune_params arm_fa726te_tune =
{
arm_9e_rtx_costs,
fa726te_sched_adjust_cost,
1,
ARM_PREFETCH_NOT_BENEFICIAL
};
>>
>> --- tmp/gcc-4.6-svn-20101116/gcc/config/arm/fa726te.md 1970-01-01 08:00:00.000000000 +0800
>> +++ gcc-4.6-svn-20101116/gcc/config/arm/fa726te.md 2010-11-25 17:06:01.877554000 +0800
>> @@ -0,0 +1,221 @@
>>
>> +(define_automaton "fa726te")
>> +(automata_option "ndfa")
>> +
>
> Why do you have an ndfa option here? Does this give you benefit with benchmarking on the FA726te core since this usually increases compile time
> as the automaton ends up searching for all possible options ?
>
Yes. the ndfa option does benefit our benchmarking, but a little. So I
remove the ndfa option here.
>
>> +;; pretend we have 2 LSUs (the second is ONLY for LDR), which can possibly
>> +;; improve code quality
>
> Full stop at the end of the comment. pretend should start with a capital P and not lower case. (Pretend)
> 2 spaces between the a full stop or a punctuation character that terminates a sentence and the start of
> the next sentence. There are a number of places in your patch where one can see such cases.
>
Fixed.
>
>> +(define_query_cpu_unit "fa726te_lsu1_pipe_e,fa726te_lsu1_pipe_w" "fa726te")
>
> You have a query_cpu_unit which you don't seem to be querying for in the backend in any form? Is there any thing else
> missing in your pipeline description or has this been put in for future use ?
>
I only use the units defined by query_cpu_unit in the fa726te.md. The
two units, fa726te_lsu1_pipe_e and
fa726te_lsu1_pipe_w, are only used for arrange the load instructions.
Because fa726te only supports one
ldr/str pipe, I use the query_cpu_unit to define another pseudo pipe
for better load instruction scheduling.
>> +;; reservation which blocks IS
>> +(define_reservation "fa726te_blockage" "(fa726te_is0+fa726te_is1)")
>
> Can you clarify the comment above ? Again the comments about sentence case and full stops hold.
>
It is used to restrict the instruction issue to one.
>
>
> cheers
> Ramana
>
>
>
>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc_4.6_svn_167325_faraday_cpu_support.patch
Type: application/octet-stream
Size: 46669 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20101202/bf04bf0b/attachment.obj>
More information about the Gcc-patches
mailing list