On Tue, 13 Sep 2011 21:44:54 +0100
Ramana Radhakrishnan <ramana.radhakrish...@linaro.org> wrote:
> On 9 September 2011 13:56, Richard Sandiford
> <richard.sandif...@linaro.org> wrote:
> > Ping for this patch:
>
> I do have a one nit on the ml bit though I must say I'm not an ML
> expert which is why I resisted for a while. The one comment that I
> have and I should have realized earlier was that the file had been
> parameterized by the core in quite a few places and I would like to
> still retain that capability.
>
> This look better ? I have retained your original logic and only
> parameterized wblatency on the core .
>
> Thoughts ? Can someone else also give the ML bits a once-over ? It
> appears to generate identical descriptions to yours.
A couple of stylistic points:
+(* The latency to use on all address register writeback dependencies. *)
+let writebackLatency whichcore =
+ match whichcore with
+ CortexA8 -> 1
+ | CortexA9 -> 1
This always returns 1? Why not just:
let writebackLatency _ = 1
Also you could write:
let writebackLatency = function
CortexA8 -> 1
| CortexA9 -> 1
which is a little more concise.
+ if (try ignore (Hashtbl.find ht (guard, latency)); false
+ with Not_found -> true)
There's no need for this construct. Use Hashtbl.mem ("membership") instead,
e.g.:
if not (Hashtbl.mem ht (guard, latency)) then ...
(I see that you're just modifying existing code, but still...)
+ let comp_fn (guard1, latency1) (guard2, latency2) =
+ if latency1 > latency2 then -1
+ else if latency1 < latency2 then 1
+ else if guard1 > guard2 then -1
+ else if guard2 > guard1 then 1
+ else 0
You can use the built-in function compare (e.g. "compare guard2 guard1") here,
instead of the if/else chain. It has exactly the semantics you need, i.e. it
returns -1/0/1 for less-than/equal/greater-than.
I have no comments on the patch algorithmically speaking.
Thanks,
Julian
