[PATCH] D139386: [RISCV][THEAD] Vendor extensions: Add 'XTheadVdot' T-Head vendor extensions

[Jiejie Rong via Phabricator via llvm-commits]

JojoR added inline comments.


================
Comment at: clang/test/CodeGen/RISCV/Vendor/THEAD/vdot-intrinsics-overloaded/vmaqa.c:5
+
+#include <riscv_vector.h>
+
----------------
jrtc27 wrote:
> I don't think T-Head intrinsics belong in riscv_vector.h, that's for V
Thanks for your comments :)

from our ISAs spec if you have seen it, they are designed based on vector v1.0,
and most rules of that like mc or intrinsic derive from RVV code, so I could copy whole from that,
but I think RISCV is base code for vendor's function if they want to reuse them like llvm code is base code for all backend. we should encourage vendor to reuse RISCV base code if they are stable and help vendor construct function easily or quickly, alright ?

What header file should we supply for this ? any suggestion ?


================
Comment at: llvm/lib/Target/RISCV/CMakeLists.txt:74
+
+add_subdirectory(Vendor)
----------------
jrtc27 wrote:
> Do we really need this? It's only a handful of TableGen files, all of which should have the extension (or family) name in them. Hopefully we're not going to have so many that we'll need this...
Thanks for your comments :)

We should make the long term planning because of RISCV ISAs allow vendor to extend their personal ISAs, alright ? :)

And for another hand, we should also help or guide vendor put their code in the the individual root space, I think it's convenience for vendor and RISCV, you can say one step to create customer's extension: "just copy a skeleton from Vendor sub-directory" :)

I believe that RISCV is open and most of vendors want to put their ISAs into community :)
you think about if there are more than 20 vendors ? is it possible ?


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:530
+//===----------------------------------------------------------------------===//
+include "Vendor/THEAD/THEAD.td"
+
----------------
jrtc27 wrote:
> We already have a block in this file for vendor extensions
Like my consideration is described above, there will be much more vendors ?
they should maintain their stuffs in some one root place ?


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:36
 class RISCVSubtarget : public RISCVGenSubtargetInfo {
+#include "Vendor/THEAD/THEADSubtarget.h"
+
----------------
jrtc27 wrote:
> ... no
Like my consideration is described above, there will be much more vendors ?
they should maintain their stuffs in some one root place ?


================
Comment at: llvm/lib/Target/RISCV/Vendor/THEAD/THEADInstrInfoV.td:1
+
+//===----------------------------------------------------------------------===//
----------------
jrtc27 wrote:
> This isn't V, don't call it V
I think I explain reason clearly above, do you agree ?


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

https://reviews.llvm.org/D139386