[PATCH] D75470: [SVE] Auto-generate builtins and header for svld1.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Big patch, I only did a first scan, but looks very reasonable in general. Just a first round of nits and 2 questions.



================
Comment at: clang/include/clang/Basic/arm_sve.td:121
+// Load one vector (scalar base)
+def SVLD1   : MInst<"svld1[_{2}]", "dPc", "csilUcUsUiUlhfd", [IsLoad]>;
----------------
This encoding, e.g, this is  "csilUcUsUiUlhfd", is such a monstrosity. It's a very efficient encoding, but of course completely unreadable.  I know there is prior art, and know that this is how it's been done, but just curious if you have given it thoughts how to do this in a normal way, a bit more c++y.  I don't want to de-rail this work, but if we are adding a new emitter, perhaps now is the time to give it a thought, so was just curious.  


================
Comment at: clang/utils/TableGen/SveEmitter.cpp:1
 //===- SveEmitter.cpp - Generate arm_sve.h for use with clang -*- C++ -*-===//
 //
----------------
I wanted to add the nit that SveEmiiter.cpp should perhaps be SVEEmitter.cpp, but then I saw at the bottom that MVE is spelled Mve, so perhaps this is fine then.


================
Comment at: clang/utils/TableGen/SveEmitter.cpp:64
+        Predicate(false), PredicatePattern(false), PrefetchOp(false),
+        Bitwidth(128), ElementBitwidth(~0U), NumVectors(1) {
+    if (!TS.empty())
----------------
why a default of 128? Will this gives problems for SVE implementions with> 128 bits?


================
Comment at: clang/utils/TableGen/SveEmitter.cpp:119
+class Intrinsic {
+  /// The Record this intrinsic was created from.
+  Record *R;
----------------
nit: for readability, perhaps don't abbreviate some of these member names? 

 R -> Record
 BaseTS -> BaseTypeSpec
 CK -> ClassKind


================
Comment at: clang/utils/TableGen/SveEmitter.cpp:263
+unsigned SVEType::getTypeFlags() const {
+  unsigned Base = 0;
+
----------------
don't need this


================
Comment at: clang/utils/TableGen/SveEmitter.cpp:267
+    switch (ElementBitwidth) {
+    case 16: Base = (unsigned)SVETypeFlags::Float16; break;
+    case 32: Base = (unsigned)SVETypeFlags::Float32; break;
----------------
just a return here 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75470