PING: [ARM] Model automodified addresses in the Cortex A8 and A9 schedulers -- NEON

PING: [ARM] Model automodified addresses in the Cortex A8 and A9 schedulers -- NEON

[Richard Sandiford]

Ping for this patch:

    http://gcc.gnu.org/ml/gcc-patches/2011-08/msg01488.html

which models address register writeback in the Cortex A8 and A9 NEON
schedulers.  (Ramana has already approved the core equivalent, thanks.)

Richard

Re: PING: [ARM] Model automodified addresses in the Cortex A8 and A9 schedulers -- NEON

[Ramana Radhakrishnan]

[-- Attachment #1: Type: text/plain, Size: 1732 bytes --]

On 9 September 2011 13:56, Richard Sandiford
<richard.sandiford@linaro.org> wrote:
> Ping for this patch:

>> This is the NEON part of the patch to handle address register writeback
>> in the Cortex A8 and A9 schedulers.  Although I can find no documentation
>> to say exactly how this is handled by the pipelines, a latency of 1
>> does seem to work well in practice, and is much easier to implement.

It sounds like this is good enough in practice.

>
>    http://gcc.gnu.org/ml/gcc-patches/2011-08/msg01488.html
>
> which models address register writeback in the Cortex A8 and A9 NEON
> schedulers.  (Ramana has already approved the core equivalent, thanks.)

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.

cheers
Ramana

gcc/
	* config/arm/neon-schedgen.ml (guard): Add Guard_writeback and
	Guard_writeback_only.
	(writebackLatency): New function.
	(collate_bypasses): Split hashtable insertion into a separate
	function.  Add address writeback dependencies for load-store
	instructions.  Use writebackLatency. Sort bypasses in order of
decreasing latency.
	(guard_fn): Handle Guard_writeback and Guard_writeback_only.
	* config/arm/cortex-a8-neon.md: Regenerated.
	* config/arm/cortex-a9-neon.md: Likewise.

[-- Attachment #2: p2 --]
[-- Type: application/octet-stream, Size: 13287 bytes --]

diff --git a/gcc/config/arm/neon-schedgen.ml b/gcc/config/arm/neon-schedgen.ml
index 3d9b044..357081d 100644
--- a/gcc/config/arm/neon-schedgen.ml
+++ b/gcc/config/arm/neon-schedgen.ml
@@ -74,6 +74,7 @@ type availability = Source of int
 		  | Dest_n_after of int * int
 
 type guard = Guard_none | Guard_only_m | Guard_only_n | Guard_only_d
+	   | Guard_writeback | Guard_writeback_only
 
 (* Reservation behaviors.  All but the last row here correspond to one
    pipeline each.  Each constructor will correspond to one
@@ -99,6 +100,11 @@ let tuneStr = function
     CortexA8 -> "cortexa8"
    | CortexA9 -> "cortexa9"
 
+(* The latency to use on all address register writeback dependencies.  *)
+let writebackLatency whichcore = 
+    match whichcore with
+    CortexA8 -> 1
+    | CortexA9 -> 1 
 
 (* This table must be kept as short as possible by conflating
    entries with the same availability behavior.
@@ -355,9 +361,16 @@ let pick_latency largest worst guards =
    of one bypass from this producer to any particular consumer listed
    in LATENCIES.)  Use a hash table to collate bypasses with the
    same latency and guard.  *)
-let collate_bypasses (producer_name, _, _, _) largest latencies core =
+let collate_bypasses (producer_name, _, resource, _) largest latencies core =
   let ht = Hashtbl.create 42 in
+  let wblatency = writebackLatency core in
   let keys = ref [] in
+    let add_latency consumer (guard, latency) =
+      if (try ignore (Hashtbl.find ht (guard, latency)); false
+	   with Not_found -> true)
+      then keys := (guard, latency) :: !keys;
+      Hashtbl.add ht (guard, latency) ((coreStr core) ^ "_" ^ consumer)
+    in
     List.iter (
       fun ((consumer, _, _, _), worst, guards) ->
         (* Find out which latency to use.  Ignoring latencies that match
@@ -369,14 +382,42 @@ let collate_bypasses (producer_name, _, _, _) largest latencies core =
         let guard_latency_opt = pick_latency largest worst guards in
           match guard_latency_opt with
             None -> ()
-          | Some (guard, latency) ->
-            begin
-              (if (try ignore (Hashtbl.find ht (guard, latency)); false
-                   with Not_found -> true) then
-                 keys := (guard, latency) :: !keys);
-              Hashtbl.add ht (guard, latency) ((coreStr core) ^ "_" ^ consumer)
-            end
+          | Some pair -> add_latency consumer pair
     ) latencies;
+    (* Add in the writeback dependencies for loads and stores.  *)
+    begin
+      match resource with
+	Ls _ ->
+	  if largest > wblatency then
+	    (* Having a writeback-only dependency decreases the latency.  *)
+	    begin
+	      (* We don't handle cases where the largest latency is
+		 greater than wblatency and where the smallest
+		 is less.  *)
+	      List.iter
+		(fun (guard, latency) -> assert (latency >= wblatency))
+		!keys;
+	      add_latency "*" (Guard_writeback_only, wblatency)
+	    end
+	  else if largest < wblatency or !keys <> [] then
+	    (* Having a writeback dependency either increases the latency
+	       or reenforces the default latency.  A bypass in only required
+	       in the latter case if there are other bypasses too.  *)
+	    add_latency "*" (Guard_writeback, wblatency)
+
+      | _ ->
+	  ()
+    end;
+    (* A comparison function that sorts keys in order of decreasing latency.
+       The guard order isn't interesting but is needed to stabilise the
+       sort.  *)
+    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
+    in
     (* The hash table now has bypasses collated so that ones with the
        same latency and guard have the same keys.  Walk through all the
        keys, extract the associated bypasses, and concatenate the names
@@ -388,7 +429,7 @@ let collate_bypasses (producer_name, _, _, _) largest latencies core =
            String.concat ",\\\n               " consumers,
            latency,
            guard)
-      ) !keys
+      ) (List.sort comp_fn !keys)
 
 (* For every producer, find the worst-case latency between it and
    *any* consumer.  Also determine (if such a thing exists) the
@@ -505,6 +546,8 @@ let guard_fn g =
     Guard_only_m -> "arm_neon_only_m_dependency"
   | Guard_only_n -> "arm_neon_only_n_dependency"
   | Guard_only_d -> "arm_neon_only_d_dependency"
+  | Guard_writeback -> "arm_writeback_dep"
+  | Guard_writeback_only -> "arm_writeback_only_dep"
   | Guard_none -> assert false
 
 (* Emit a define_bypass for each bypass.  *)
diff --git a/gcc/config/arm/cortex-a8-neon.md b/gcc/config/arm/cortex-a8-neon.md
index 03f52b2..9f9c704 100644
--- a/gcc/config/arm/cortex-a8-neon.md
+++ b/gcc/config/arm/cortex-a8-neon.md
@@ -638,6 +638,18 @@
                cortex_a8_neon_fp_vrecps_vrsqrts_ddd,\
                cortex_a8_neon_fp_vrecps_vrsqrts_qqq")
 
+(define_bypass 1 "cortex_a8_neon_vld3_vld4_all_lanes"
+               "cortex_a8_*"
+               "arm_writeback_only_dep")
+
+(define_bypass 1 "cortex_a8_neon_vst3_vst4_lane"
+               "cortex_a8_*"
+               "arm_writeback_dep")
+
+(define_bypass 1 "cortex_a8_neon_vst1_vst2_lane"
+               "cortex_a8_*"
+               "arm_writeback_dep")
+
 (define_bypass 5 "cortex_a8_neon_vld3_vld4_lane"
                "cortex_a8_neon_int_1,\
                cortex_a8_neon_int_4,\
@@ -652,6 +664,10 @@
                cortex_a8_neon_fp_vrecps_vrsqrts_ddd,\
                cortex_a8_neon_fp_vrecps_vrsqrts_qqq")
 
+(define_bypass 1 "cortex_a8_neon_vld3_vld4_lane"
+               "cortex_a8_*"
+               "arm_writeback_only_dep")
+
 (define_bypass 3 "cortex_a8_neon_vld1_vld2_lane"
                "cortex_a8_neon_int_1,\
                cortex_a8_neon_int_4,\
@@ -666,6 +682,26 @@
                cortex_a8_neon_fp_vrecps_vrsqrts_ddd,\
                cortex_a8_neon_fp_vrecps_vrsqrts_qqq")
 
+(define_bypass 1 "cortex_a8_neon_vld1_vld2_lane"
+               "cortex_a8_*"
+               "arm_writeback_only_dep")
+
+(define_bypass 1 "cortex_a8_neon_vst3_vst4"
+               "cortex_a8_*"
+               "arm_writeback_dep")
+
+(define_bypass 1 "cortex_a8_neon_vst2_4_regs_vst3_vst4"
+               "cortex_a8_*"
+               "arm_writeback_dep")
+
+(define_bypass 1 "cortex_a8_neon_vst1_3_4_regs"
+               "cortex_a8_*"
+               "arm_writeback_dep")
+
+(define_bypass 1 "cortex_a8_neon_vst1_1_2_regs_vst2_2_regs"
+               "cortex_a8_*"
+               "arm_writeback_dep")
+
 (define_bypass 4 "cortex_a8_neon_vld3_vld4"
                "cortex_a8_neon_int_1,\
                cortex_a8_neon_int_4,\
@@ -680,6 +716,10 @@
                cortex_a8_neon_fp_vrecps_vrsqrts_ddd,\
                cortex_a8_neon_fp_vrecps_vrsqrts_qqq")
 
+(define_bypass 1 "cortex_a8_neon_vld3_vld4"
+               "cortex_a8_*"
+               "arm_writeback_only_dep")
+
 (define_bypass 3 "cortex_a8_neon_vld2_4_regs"
                "cortex_a8_neon_int_1,\
                cortex_a8_neon_int_4,\
@@ -694,6 +734,10 @@
                cortex_a8_neon_fp_vrecps_vrsqrts_ddd,\
                cortex_a8_neon_fp_vrecps_vrsqrts_qqq")
 
+(define_bypass 1 "cortex_a8_neon_vld2_4_regs"
+               "cortex_a8_*"
+               "arm_writeback_only_dep")
+
 (define_bypass 2 "cortex_a8_neon_vld2_2_regs_vld1_vld2_all_lanes"
                "cortex_a8_neon_int_1,\
                cortex_a8_neon_int_4,\
@@ -708,6 +752,10 @@
                cortex_a8_neon_fp_vrecps_vrsqrts_ddd,\
                cortex_a8_neon_fp_vrecps_vrsqrts_qqq")
 
+(define_bypass 1 "cortex_a8_neon_vld2_2_regs_vld1_vld2_all_lanes"
+               "cortex_a8_*"
+               "arm_writeback_only_dep")
+
 (define_bypass 2 "cortex_a8_neon_vld1_3_4_regs"
                "cortex_a8_neon_int_1,\
                cortex_a8_neon_int_4,\
@@ -722,6 +770,14 @@
                cortex_a8_neon_fp_vrecps_vrsqrts_ddd,\
                cortex_a8_neon_fp_vrecps_vrsqrts_qqq")
 
+(define_bypass 1 "cortex_a8_neon_vld1_3_4_regs"
+               "cortex_a8_*"
+               "arm_writeback_only_dep")
+
+(define_bypass 1 "cortex_a8_neon_vld1_1_2_regs"
+               "cortex_a8_*"
+               "arm_writeback_only_dep")
+
 (define_bypass 1 "cortex_a8_neon_vld1_1_2_regs"
                "cortex_a8_neon_int_1,\
                cortex_a8_neon_int_4,\
@@ -736,6 +792,14 @@
                cortex_a8_neon_fp_vrecps_vrsqrts_ddd,\
                cortex_a8_neon_fp_vrecps_vrsqrts_qqq")
 
+(define_bypass 1 "cortex_a8_neon_str"
+               "cortex_a8_*"
+               "arm_writeback_dep")
+
+(define_bypass 1 "cortex_a8_neon_ldr"
+               "cortex_a8_*"
+               "arm_writeback_dep")
+
 (define_bypass 0 "cortex_a8_neon_ldr"
                "cortex_a8_neon_int_1,\
                cortex_a8_neon_int_4,\
@@ -1309,4 +1373,3 @@
                cortex_a8_neon_fp_vmla_qqq,\
                cortex_a8_neon_fp_vrecps_vrsqrts_ddd,\
                cortex_a8_neon_fp_vrecps_vrsqrts_qqq")
-
diff --git a/gcc/config/arm/cortex-a9-neon.md b/gcc/config/arm/cortex-a9-neon.md
index 2e8ec9b..ff13145 100644
--- a/gcc/config/arm/cortex-a9-neon.md
+++ b/gcc/config/arm/cortex-a9-neon.md
@@ -563,6 +563,18 @@
                cortex_a9_neon_fp_vrecps_vrsqrts_ddd,\
                cortex_a9_neon_fp_vrecps_vrsqrts_qqq")
 
+(define_bypass 1 "cortex_a9_neon_vld3_vld4_all_lanes"
+               "cortex_a9_*"
+               "arm_writeback_only_dep")
+
+(define_bypass 1 "cortex_a9_neon_vst3_vst4_lane"
+               "cortex_a9_*"
+               "arm_writeback_dep")
+
+(define_bypass 1 "cortex_a9_neon_vst1_vst2_lane"
+               "cortex_a9_*"
+               "arm_writeback_dep")
+
 (define_bypass 5 "cortex_a9_neon_vld3_vld4_lane"
                "cortex_a9_neon_int_1,\
                cortex_a9_neon_int_4,\
@@ -577,6 +589,10 @@
                cortex_a9_neon_fp_vrecps_vrsqrts_ddd,\
                cortex_a9_neon_fp_vrecps_vrsqrts_qqq")
 
+(define_bypass 1 "cortex_a9_neon_vld3_vld4_lane"
+               "cortex_a9_*"
+               "arm_writeback_only_dep")
+
 (define_bypass 3 "cortex_a9_neon_vld1_vld2_lane"
                "cortex_a9_neon_int_1,\
                cortex_a9_neon_int_4,\
@@ -591,6 +607,26 @@
                cortex_a9_neon_fp_vrecps_vrsqrts_ddd,\
                cortex_a9_neon_fp_vrecps_vrsqrts_qqq")
 
+(define_bypass 1 "cortex_a9_neon_vld1_vld2_lane"
+               "cortex_a9_*"
+               "arm_writeback_only_dep")
+
+(define_bypass 1 "cortex_a9_neon_vst3_vst4"
+               "cortex_a9_*"
+               "arm_writeback_dep")
+
+(define_bypass 1 "cortex_a9_neon_vst2_4_regs_vst3_vst4"
+               "cortex_a9_*"
+               "arm_writeback_dep")
+
+(define_bypass 1 "cortex_a9_neon_vst1_3_4_regs"
+               "cortex_a9_*"
+               "arm_writeback_dep")
+
+(define_bypass 1 "cortex_a9_neon_vst1_1_2_regs_vst2_2_regs"
+               "cortex_a9_*"
+               "arm_writeback_dep")
+
 (define_bypass 4 "cortex_a9_neon_vld3_vld4"
                "cortex_a9_neon_int_1,\
                cortex_a9_neon_int_4,\
@@ -605,6 +641,10 @@
                cortex_a9_neon_fp_vrecps_vrsqrts_ddd,\
                cortex_a9_neon_fp_vrecps_vrsqrts_qqq")
 
+(define_bypass 1 "cortex_a9_neon_vld3_vld4"
+               "cortex_a9_*"
+               "arm_writeback_only_dep")
+
 (define_bypass 3 "cortex_a9_neon_vld2_4_regs"
                "cortex_a9_neon_int_1,\
                cortex_a9_neon_int_4,\
@@ -619,6 +659,10 @@
                cortex_a9_neon_fp_vrecps_vrsqrts_ddd,\
                cortex_a9_neon_fp_vrecps_vrsqrts_qqq")
 
+(define_bypass 1 "cortex_a9_neon_vld2_4_regs"
+               "cortex_a9_*"
+               "arm_writeback_only_dep")
+
 (define_bypass 2 "cortex_a9_neon_vld2_2_regs_vld1_vld2_all_lanes"
                "cortex_a9_neon_int_1,\
                cortex_a9_neon_int_4,\
@@ -633,6 +677,10 @@
                cortex_a9_neon_fp_vrecps_vrsqrts_ddd,\
                cortex_a9_neon_fp_vrecps_vrsqrts_qqq")
 
+(define_bypass 1 "cortex_a9_neon_vld2_2_regs_vld1_vld2_all_lanes"
+               "cortex_a9_*"
+               "arm_writeback_only_dep")
+
 (define_bypass 2 "cortex_a9_neon_vld1_3_4_regs"
                "cortex_a9_neon_int_1,\
                cortex_a9_neon_int_4,\
@@ -647,6 +695,14 @@
                cortex_a9_neon_fp_vrecps_vrsqrts_ddd,\
                cortex_a9_neon_fp_vrecps_vrsqrts_qqq")
 
+(define_bypass 1 "cortex_a9_neon_vld1_3_4_regs"
+               "cortex_a9_*"
+               "arm_writeback_only_dep")
+
+(define_bypass 1 "cortex_a9_neon_vld1_1_2_regs"
+               "cortex_a9_*"
+               "arm_writeback_only_dep")
+
 (define_bypass 1 "cortex_a9_neon_vld1_1_2_regs"
                "cortex_a9_neon_int_1,\
                cortex_a9_neon_int_4,\
@@ -661,6 +717,14 @@
                cortex_a9_neon_fp_vrecps_vrsqrts_ddd,\
                cortex_a9_neon_fp_vrecps_vrsqrts_qqq")
 
+(define_bypass 1 "cortex_a9_neon_str"
+               "cortex_a9_*"
+               "arm_writeback_dep")
+
+(define_bypass 1 "cortex_a9_neon_ldr"
+               "cortex_a9_*"
+               "arm_writeback_dep")
+
 (define_bypass 0 "cortex_a9_neon_ldr"
                "cortex_a9_neon_int_1,\
                cortex_a9_neon_int_4,\

Re: PING: [ARM] Model automodified addresses in the Cortex A8 and A9 schedulers -- NEON

[Richard Sandiford]

Ramana Radhakrishnan <ramana.radhakrishnan@linaro.org> writes:
> 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 .

Looks good, thanks.

Richard

Re: PING: [ARM] Model automodified addresses in the Cortex A8 and A9 schedulers -- NEON

[Ramana Radhakrishnan]

On 14 September 2011 08:47, Richard Sandiford
<richard.sandiford@linaro.org> wrote:
> Ramana Radhakrishnan <ramana.radhakrishnan@linaro.org> writes:
>> 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 .
>
> Looks good, thanks.


OK - please apply if no one objects in 24 hours.

Ramana
>
> Richard
>

Re: PING: [ARM] Model automodified addresses in the Cortex A8 and A9 schedulers -- NEON

[Julian Brown]

On Tue, 13 Sep 2011 21:44:54 +0100
Ramana Radhakrishnan <ramana.radhakrishnan@linaro.org> wrote:

> On 9 September 2011 13:56, Richard Sandiford
> <richard.sandiford@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