This is an archive of the discontinued LLVM Phabricator instance.

[llvm][SVE] Reg + reg addressing mode for LD1RO.
ClosedPublic

Authored by fpetrogalli on May 28 2020, 10:04 AM.

Diff Detail

Event Timeline

fpetrogalli created this revision.May 28 2020, 10:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2020, 10:04 AM
efriedma added inline comments.May 28 2020, 11:52 AM
llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
1958–1959

Is there some reason to pass AArch64ld1ro as an argument, as opposed to hardcoding it in sve_mem_ldor_ss?

llvm/lib/Target/AArch64/SVEInstrFormats.td
7709

Is the AddedComplexity here actually necessary?

llvm/test/CodeGen/AArch64/sve-intrinsics-ld1ro-addressing-mode-reg-reg.ll
81

I'd like to see a couple testcases where the pattern doesn't match; wrong shift, or subtraction, or something like that

fpetrogalli marked 5 inline comments as done.

Thank you for the review @efriedma.

Francesco

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
1958–1959

I think it is just a convention. The defs of these custom SDNodes are in AArch64SVEInstrInfo.td, not in its include file SVEInstrFormats.td, which is where the multiclass is defined.

For example, see:

frapet01@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>;
llvm/lib/Target/AArch64/SVEInstrFormats.td
7709

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.

llvm/test/CodeGen/AArch64/sve-intrinsics-ld1ro-addressing-mode-reg-reg.ll
81

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?

efriedma accepted this revision.Jun 16 2020, 2:22 PM

LGTM

llvm/test/CodeGen/AArch64/sve-intrinsics-ld1ro-addressing-mode-reg-reg.ll
81

D80738 covers the general possibility that there is no match, but it doesn't really verify that am_sve_regreg_lsl1 is doing the right thing in non-trivial cases. I guess we have coverage from other instructions, though.

This revision is now accepted and ready to land.Jun 16 2020, 2:22 PM

Added the bfloat test case that was missing.

This revision was automatically updated to reflect the committed changes.