This patch implements the T-Head vendor extensions (XTHeadVdot),
which is documented here, it's based on standard vector extension v1.0:
https://github.com/T-head-Semi/thead-extension-spec
Paths
| Differential D139386
[RISCV] Implement assembler support for XTHeadVdot ClosedPublic Authored by JojoR on Dec 5 2022, 6:37 PM.
Details Summary This patch implements the T-Head vendor extensions (XTHeadVdot), https://github.com/T-head-Semi/thead-extension-spec
Diff Detail Event TimelineComment Actions Aside from the maintainability question of having two similar but conflicting scalable vector extensions in the backend, this needs splitting up into separate MC, CodeGen and Clang patches like any other extension.
Comment Actions I have not looked at the change at all (yet), but a couple of high level points.
Comment Actions
Thanks for your comments and I will attend if I am free at that time :) Comment Actions Just to document the discussion we had in the RISC-V sync call yesterday, my understanding is that we concluded there aren't objections to reviewing and merging support for this extension once reviewers are happy with code quality, on the basis that the extension is relatively small, there are people willing to support it, and it's based on v1.0 vector ISA rather than 0.7 (which would trigger a much larger discussion).
JojoR marked 4 inline comments as done. Comment Actions I am confused about the failures of CI testing and checked that is AddressSanitizer-Unit of x86, Comment Actions
If these failures are apparently irrelevant to your changes, you can just ignore them since they may be caused by other landed patches. Comment Actions If this is some new T-Head extension based on the ratified V 1.0 rather than the 0.7.1 draft implemented in existing T-Head hardware then the compatibility points I made can be ignored. I would prefer that llvm/test remain flatter though and not have Vendor/THEAD/vdot, just use a single level with xtheadvdot as the name. In the case of MC I'd go one step further and say, given it's just a single file (that should conform to the -(in)valid.s convention that already exists there) it should be xtheadvdot-valid.s or whatever (just copy the Ventana approach).
JojoR marked an inline comment as done. Comment Actions It seems we have split Clang changes off, but this patch still mix MC and CodeGen changes.
Comment Actions Next step to make progress here is to strip out everything which isn't related to MC (assembler and disassembler). The intrinsic codegen changes should be a separate patch.
This revision is now accepted and ready to land.Dec 20 2022, 2:16 AM Comment Actions Reverse ping on this - @JojoR are there outstanding changes you have planned for this, or is it ready to go on from your perspective. I _think_ the review comments were all addressed and of course it got a LGTM from eop.
Revision Contents
Diff 481237 clang/include/clang/Basic/riscv_thead_vector.td
clang/include/clang/Basic/riscv_vector.td
clang/include/clang/Support/RISCVVIntrinsicUtils.h
clang/lib/Support/RISCVVIntrinsicUtils.cpp
clang/test/CodeGen/RISCV/Vendor/THEAD/vdot-intrinsics-overloaded/vmaqa.c
clang/test/CodeGen/RISCV/Vendor/THEAD/vdot-intrinsics/vmaqa.c
clang/test/Preprocessor/riscv-target-thead-features.c
llvm/include/llvm/IR/IntrinsicsRISCV.td
llvm/include/llvm/IR/IntrinsicsRISCV_THEAD.td
llvm/lib/Support/RISCVISAInfo.cpp
llvm/lib/Target/RISCV/CMakeLists.txt
llvm/lib/Target/RISCV/RISCV.td
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
llvm/lib/Target/RISCV/RISCVSubtarget.h
llvm/lib/Target/RISCV/Vendor/CMakeLists.txt
llvm/lib/Target/RISCV/Vendor/THEAD/THEAD.td
llvm/lib/Target/RISCV/Vendor/THEAD/THEADInstrInfoV.td
llvm/lib/Target/RISCV/Vendor/THEAD/THEADInstrInfoVPseudo.td
llvm/lib/Target/RISCV/Vendor/THEAD/THEADSubtarget.h
llvm/test/CodeGen/RISCV/Vendor/THEAD/attributes.ll
llvm/test/CodeGen/RISCV/Vendor/THEAD/vdot/vmaqa.ll
llvm/test/CodeGen/RISCV/Vendor/THEAD/vdot/vmaqasu.ll
llvm/test/CodeGen/RISCV/Vendor/THEAD/vdot/vmaqau.ll
llvm/test/CodeGen/RISCV/Vendor/THEAD/vdot/vmaqaus.ll
llvm/test/MC/RISCV/Vendor/THEAD/vdot/vmaqa.s
|
Maybe this "only" has some clear meaning to those familiar to V but I have no clue from reading this how it differs from "q". From reading the code it seems to just be q without affecting LMUL; maybe that should be a non-primitive type transformer or something so we don't end up expanding out the whole orthogonal space.