Hi Charles,

Good to see these intrinsics being brought into the modern world :)
Some style comments inline.

On 18/09/14 20:38, Charles Baylis wrote:
This patch adds new patterns and builtins to represent single lane structure
loads instructions, which will be used to implement the vld[234](q?)_lane_*
intrinsics.

Tested (with the rest of the patch series) with make check on aarch64-oe-linux
with qemu, and also causes no regressions in clyon's NEON intrinsics tests.

<DATE>  Charles Baylis  <charles.bay...@linaro.org>
        * config/aarch64/aarch64-builtins.c
        (aarch64_types_loadstruct_lane_qualifiers): Define.
        * config/aarch64/aarch64-simd-builtins.def (ld2_lane, ld3_lane,
        ld4_lane): New builtins.
        * config/aarch64/aarch64-simd.md (vec_load_lanesoi_lane<mode>): New
        pattern.
        (vec_load_lanesci_lane<mode>): Likewise.
        (vec_load_lanesxi_lane<mode>): Likewise.
        (aarch64_ld2_lane<VQ:mode>): New expand.
        (aarch64_ld3_lane<VQ:mode>): Likewise.
        (aarch64_ld4_lane<VQ:mode>): Likewise.

This is missing an entry for the config/aarch64/aarch64.md hunk.

diff --git a/gcc/config/aarch64/aarch64-simd.md 
b/gcc/config/aarch64/aarch64-simd.md
index 493e886..f6c4018 100644
--- a/gcc/config/aarch64/aarch64-simd.md
+++ b/gcc/config/aarch64/aarch64-simd.md
@@ -4003,6 +4003,18 @@
    [(set_attr "type" "neon_load2_2reg<q>")]
  )
+(define_insn "vec_load_lanesoi_lane<mode>"
+  [(set (match_operand:OI 0 "register_operand" "=w")
+       (unspec:OI [(match_operand:<V_TWO_ELEM> 1 "aarch64_simd_struct_operand" 
"Utv")
+                   (match_operand:OI 2 "register_operand" "0")
+                   (match_operand:SI 3 "immediate_operand" "i")
+                   (unspec:VQ [(const_int 0)] UNSPEC_VSTRUCTDUMMY) ]
+                  UNSPEC_LD2_LANE))]
+  "TARGET_SIMD"
+  "ld2\\t{%S0.<Vetype> - %T0.<Vetype>}[%3], %1"
+  [(set_attr "type" "neon_load2_one_lane<q>")]
+)

The VQ mode iterator goes over the 128-wide modes so the "type" attribute here will always be neon_load2_one_lane_q. Using the <q> mode attribute is still correct but personally I think it makes it just that little bit harder to figure out for a newbie who will have to open iterators.md to figure out the meaning of it, or for someone who's not sure whether the 'q' is added with an underscore or without. I would just use neon_load2_one_lane_q.

+(define_insn "vec_load_lanesci_lane<mode>"
+  [(set (match_operand:CI 0 "register_operand" "=w")
+       (unspec:CI [(match_operand:<V_THREE_ELEM> 1 "aarch64_simd_struct_operand" 
"Utv")
+                   (match_operand:CI 2 "register_operand" "0")
+                   (match_operand:SI 3 "immediate_operand" "i")
+                   (unspec:VQ [(const_int 0)] UNSPEC_VSTRUCTDUMMY)]
+                  UNSPEC_LD3_LANE))]
+  "TARGET_SIMD"
+  "ld3\\t{%S0.<Vetype> - %U0.<Vetype>}[%3], %1"
+  [(set_attr "type" "neon_load3_one_lane<q>")]
+)

Likewise.

+(define_insn "vec_load_lanesxi_lane<mode>"
+  [(set (match_operand:XI 0 "register_operand" "=w")
+       (unspec:XI [(match_operand:<V_FOUR_ELEM> 1 "aarch64_simd_struct_operand" 
"Utv")
+                   (match_operand:XI 2 "register_operand" "0")
+                   (match_operand:SI 3 "immediate_operand" "i")
+                   (unspec:VQ [(const_int 0)] UNSPEC_VSTRUCTDUMMY)]
+                  UNSPEC_LD4_LANE))]
+  "TARGET_SIMD"
+  "ld4\\t{%S0.<Vetype> - %V0.<Vetype>}[%3], %1"
+  [(set_attr "type" "neon_load4_one_lane<q>")]
+)

Same here.

+(define_expand "aarch64_ld2_lane<VQ:mode>"
+  [(match_operand:OI 0 "register_operand" "=w")
+       (match_operand:DI 1 "register_operand" "w")
+       (match_operand:OI 2 "register_operand" "0")
+       (match_operand:SI 3 "immediate_operand" "i")
+       (unspec:VQ [(const_int 0)] UNSPEC_VSTRUCTDUMMY)]
+  "TARGET_SIMD"
+{
+  enum machine_mode mode = <V_TWO_ELEM>mode;
+  rtx mem = gen_rtx_MEM (mode, operands[1]);
+  operands[3] = GEN_INT (ENDIAN_LANE_N (<MODE>mode, INTVAL (operands[3])));
+
+  emit_insn (gen_vec_load_lanesoi_lane<VQ:mode> (operands[0],
+                                                 mem,
+                                                 operands[2],
+                                                 operands[3]));
+  DONE;
+})

I think saying <VQ:mode> is redundant since VQ is the only mode iterator in the pattern.
Just <mode> should work, right?

+
+(define_expand "aarch64_ld3_lane<VQ:mode>"
+  [(match_operand:CI 0 "register_operand" "=w")
+       (match_operand:DI 1 "register_operand" "w")
+       (match_operand:CI 2 "register_operand" "0")
+       (match_operand:SI 3 "immediate_operand" "i")
+       (unspec:VQ [(const_int 0)] UNSPEC_VSTRUCTDUMMY)]
+  "TARGET_SIMD"
+{
+  enum machine_mode mode = <V_THREE_ELEM>mode;
+  rtx mem = gen_rtx_MEM (mode, operands[1]);
+  operands[3] = GEN_INT (ENDIAN_LANE_N (<MODE>mode, INTVAL (operands[3])));
+
+  emit_insn (gen_vec_load_lanesci_lane<VQ:mode> (operands[0],
+                                                 mem,
+                                                 operands[2],
+                                                 operands[3]));
+  DONE;
+})
Likewise.

+
+(define_expand "aarch64_ld4_lane<VQ:mode>"
+  [(match_operand:XI 0 "register_operand" "=w")
+       (match_operand:DI 1 "register_operand" "w")
+       (match_operand:XI 2 "register_operand" "0")
+       (match_operand:SI 3 "immediate_operand" "i")
+       (unspec:VQ [(const_int 0)] UNSPEC_VSTRUCTDUMMY)]
+  "TARGET_SIMD"
+{
+  enum machine_mode mode = <V_FOUR_ELEM>mode;
+  rtx mem = gen_rtx_MEM (mode, operands[1]);
+  operands[3] = GEN_INT (ENDIAN_LANE_N (<MODE>mode, INTVAL (operands[3])));
+
+  emit_insn (gen_vec_load_lanesxi_lane<VQ:mode> (operands[0],
+                                                 mem,
+                                                 operands[2],
+                                                 operands[3]));
+  DONE;
+})
+

Likewise.

+
  ;; Expanders for builtins to extract vector registers from large
  ;; opaque integer modes.
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index c60038a..ea924ab 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -92,6 +92,9 @@
      UNSPEC_LD2
      UNSPEC_LD3
      UNSPEC_LD4
+    UNSPEC_LD2_LANE
+    UNSPEC_LD3_LANE
+    UNSPEC_LD4_LANE
      UNSPEC_MB
      UNSPEC_NOP
      UNSPEC_PRLG_STK


Reply via email to