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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

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 at: clang/include/clang/Basic/riscvTHEAD_vector.td:1
+
+multiclass THVdotOutOp1Op2BuiltinSet<string intrinsic_name, string type_range,
----------------
You have a lot of files with a blank first line and no header


================
Comment at: clang/include/clang/Basic/riscv_vector.td:65
+//   h: computes a vector type identical to what 'v' computes except for the
+//      element type which is four times as wide as the element type only of 'v'
 //   m: computes a vector type identical to what 'v' computes except for the
----------------
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.


================
Comment at: clang/include/clang/Basic/riscv_vector.td:2396
+
+include "clang/Basic/riscvTHEAD_vector.td"
----------------
riscv_thead_vector maybe? This is really ugly


================
Comment at: clang/test/CodeGen/RISCV/Vendor/THEAD/vdot-intrinsics-overloaded/vmaqa.c:3
+// REQUIRES: riscv-registered-target
+// RUN: %clang_cc1 -triple riscv64 -target-feature +v -disable-O0-optnone -emit-llvm %s -o - | opt -S -passes=mem2reg | FileCheck %s
+
----------------
These tests are using +v not +xtheadvdot and demonstrates that your intrinsics aren't predicted on the right thing, since this should give an error


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


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCVxTHEAD.td:1
+
+let TargetPrefix = "riscv" in {
----------------
Xthead is the standard capitalisation for extensions when not doing lowercase


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCVxTHEAD.td:4
+
+  class THVdotTernaryWideMasked
+        : Intrinsic< [llvm_anyvector_ty],
----------------
TH_ given it's "th."


================
Comment at: llvm/lib/Target/RISCV/CMakeLists.txt:74
+
+add_subdirectory(Vendor)
----------------
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...


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:530
+//===----------------------------------------------------------------------===//
+include "Vendor/THEAD/THEAD.td"
+
----------------
We already have a block in this file for vendor extensions


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:1715
 
+include "Vendor/THEAD/THEADInstrInfoV.td"
 include "RISCVInstrInfoVPseudos.td"
----------------
Put this at the top level, don't tie it to V


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:1717
 include "RISCVInstrInfoVPseudos.td"
+include "Vendor/THEAD/THEADInstrInfoVPseudo.td"
----------------
Include this in the InstrInfo file


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:36
 class RISCVSubtarget : public RISCVGenSubtargetInfo {
+#include "Vendor/THEAD/THEADSubtarget.h"
+
----------------
... no


================
Comment at: llvm/lib/Target/RISCV/Vendor/CMakeLists.txt:2
+
+add_subdirectory(THEAD)
----------------
As for the Vendor directory itself... seems wholly unnecessary


================
Comment at: llvm/lib/Target/RISCV/Vendor/THEAD/THEAD.td:2
+
+def FeatureTHeadVdot
+    : SubtargetFeature<"xtheadvdot", "HasTHeadVdot", "true",
----------------
VDot seems more natural


================
Comment at: llvm/lib/Target/RISCV/Vendor/THEAD/THEADInstrInfoV.td:1
+
+//===----------------------------------------------------------------------===//
----------------
This isn't V, don't call it V


================
Comment at: llvm/lib/Target/RISCV/Vendor/THEAD/THEADInstrInfoV.td:5
+//===----------------------------------------------------------------------===//
+class THInstVdotVV<bits<6> funct6, RISCVVFormat opv, dag outs, dag ins,
+                   string opcodestr, string argstr>
----------------
Unclear if it should be tied to V or use its own classes


================
Comment at: llvm/lib/Target/RISCV/Vendor/THEAD/THEADSubtarget.h:6
+  bool hasTHeadVdot() const { return HasTHeadVdot; }
\ No newline at end of file

----------------
Fix


================
Comment at: llvm/test/CodeGen/RISCV/Vendor/THEAD/attributes.ll:5
+
+; RV64XTHEADVDOT: .attribute 5, "rv64i2p0_xtheadvdot1p0"
+
----------------
xventanacondops is being tested in the normal attributes.ll


================
Comment at: llvm/test/CodeGen/RISCV/Vendor/THEAD/vdot/vmaqa.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: sed 's/iXLen/i32/g' %s | llc -mtriple=riscv32 -mattr=+v,+xtheadvdot \
+; RUN:   -verify-machineinstrs | FileCheck %s
----------------
+v should be incompatible


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

https://reviews.llvm.org/D139386