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,

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.


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


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

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

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


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


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

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



cheers
Ramana






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