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

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added inline comments.


================
Comment at: clang/include/clang/Basic/arm_sve.td:121
+// Load one vector (scalar base)
+def SVLD1   : MInst<"svld1[_{2}]", "dPc", "csilUcUsUiUlhfd", [IsLoad]>;
----------------
SjoerdMeijer wrote:
> 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.  
Haha, its a bit of a monstrosity indeed. The only thing I can think of here would be having something like:
```class TypeSpecs<list<string> val> {
  list<string> v = val;
}
def All_Int_Float_Ty : TypeSpecs<["c", "s", "i", "l", "Uc", "Us", "Ul", "h", "f", "d">;

def SVLD1 : Minst<"svld1[_{2}]", "dPc", All_Int_Float_Ty, [IsLoad]>;```

But I suspect this gets a bit awkward because of the many permutations, I count more than 40. Not sure if that would really improve the readability.


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


================
Comment at: clang/utils/TableGen/SveEmitter.cpp:119
+class Intrinsic {
+  /// The Record this intrinsic was created from.
+  Record *R;
----------------
SjoerdMeijer wrote:
> nit: for readability, perhaps don't abbreviate some of these member names? 
> 
>  R -> Record
>  BaseTS -> BaseTypeSpec
>  CK -> ClassKind
`Record` and `ClassKind` are also the names of the enum though.
Perhaps I can rename CK to `Class`?


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


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

https://reviews.llvm.org/D75470