[PATCH] D80741: [llvm][SVE] Reg + reg addressing mode for LD1RO.

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:1922
   defm LD1RO_D_IMM  : sve_mem_ldor_si<0b11, "ld1rod", Z_d, ZPR64, nxv2i64, nxv2i1,  AArch64ld1ro>;
-  defm LD1RO_B      : sve_mem_ldor_ss<0b00, "ld1rob", Z_b, ZPR8,  GPR64NoXZRshifted8>;
-  defm LD1RO_H      : sve_mem_ldor_ss<0b01, "ld1roh", Z_h, ZPR16, GPR64NoXZRshifted16>;
-  defm LD1RO_W      : sve_mem_ldor_ss<0b10, "ld1row", Z_s, ZPR32, GPR64NoXZRshifted32>;
-  defm LD1RO_D      : sve_mem_ldor_ss<0b11, "ld1rod", Z_d, ZPR64, GPR64NoXZRshifted64>;
+  defm LD1RO_B      : sve_mem_ldor_ss<0b00, "ld1rob", Z_b, ZPR8,  GPR64NoXZRshifted8,  nxv16i8, nxv16i1, AArch64ld1ro, am_sve_regreg_lsl0>;
+  defm LD1RO_H      : sve_mem_ldor_ss<0b01, "ld1roh", Z_h, ZPR16, GPR64NoXZRshifted16, nxv8i16, nxv8i1,  AArch64ld1ro, am_sve_regreg_lsl1>;
----------------
efriedma wrote:
> Is there some reason to pass AArch64ld1ro as an argument, as opposed to hardcoding it in sve_mem_ldor_ss?
I think it is just a convention. The defs of these custom `SDNode`s are in `AArch64SVEInstrInfo.td`, not in its include file `SVEInstrFormats.td`, which is where the multiclass is defined.

For example, see:

```
frapet01 at man-08:~/projects/upstream-clang/llvm-project/llvm/lib/Target/AArch64 (master)$ grep AArch64ld1 *.td | head -n 30
AArch64SVEInstrInfo.td:def AArch64ld1  : SDNode<"AArch64ISD::LD1",    SDT_AArch64_LD1, [SDNPHasChain, SDNPMayLoad, SDNPOptInGlue]>;
AArch64SVEInstrInfo.td:def AArch64ld1s : SDNode<"AArch64ISD::LD1S",   SDT_AArch64_LD1, [SDNPHasChain, SDNPMayLoad, SDNPOptInGlue]>;
AArch64SVEInstrInfo.td:def AArch64ld1rq : SDNode<"AArch64ISD::LD1RQ",  SDT_AArch64_LD1Replicate, [SDNPHasChain, SDNPMayLoad]>;
AArch64SVEInstrInfo.td:def AArch64ld1ro : SDNode<"AArch64ISD::LD1RO",  SDT_AArch64_LD1Replicate, [SDNPHasChain, SDNPMayLoad]>;

[...]

AArch64SVEInstrInfo.td:  defm GLD1B_S    : sve_mem_32b_gld_vi_32_ptrs<0b0010, "ld1b",    imm0_31, AArch64ld1_gather_imm,    nxv4i8>;
AArch64SVEInstrInfo.td:  defm GLD1SH_S   : sve_mem_32b_gld_vi_32_ptrs<0b0100, "ld1sh",   uimm5s2, AArch64ld1s_gather_imm,   nxv4i16>;
AArch64SVEInstrInfo.td:  defm GLD1H_S    : sve_mem_32b_gld_vi_32_ptrs<0b0110, "ld1h",    uimm5s2, AArch64ld1_gather_imm,    nxv4i16>;

```


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:7709
+
+  let AddedComplexity = 1 in {
+    def : Pat<(Ty (Ld1ro (PredTy PPR3bAny:$gp), (AddrCP GPR64sp:$base, gprty:$offset))),
----------------
efriedma wrote:
> Is the AddedComplexity here actually necessary?
Ah right, not needed in this patch. It will probably be needed when we introduce the reg+imm optimization. In any case, I have removed it from here. Thank you for the head out.


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-ld1ro-addressing-mode-reg-reg.ll:80
+  ret <vscale x 2 x double> %load
+}
+
----------------
efriedma wrote:
> I'd like to see a couple testcases where the pattern doesn't match; wrong shift, or subtraction, or something like that
Isn't this covered by the tests in https://reviews.llvm.org/D80738? Those tests are saying: "default to `[xBASE]` if you cannot figure out an optimal addressing mode supported by the instruction". Or am I missing something?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80741/new/

https://reviews.llvm.org/D80741