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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
