[PATCH] D95016: [Clang][RISCV] Add custom TableGen backend for riscv-vector intrinsics.

[Zakk Chen via Phabricator via cfe-commits]

khchen added inline comments.


================
Comment at: clang/include/clang/Basic/riscv_vector.td:56
+//
+//   e: type of "t" as is (identity)
+//   v: computes a vector type whose element type is "t" for the current LMUL
----------------
jrtc27 wrote:
> khchen wrote:
> > jrtc27 wrote:
> > > Do we really need to invent an esoteric DSL?
> > I think this is different design choose.
> > Current design is based on https://repo.hca.bsc.es/gitlab/rferrer/llvm-epi/-/blob/EPI/clang/include/clang/Basic/epi_builtins.td, personally I think it makes td file more simpler.
> > 
> > Of course we can make td file more complex little bit and list all legal type and combination like https://github.com/isrc-cas/rvv-llvm/blob/rvv-iscas/clang/include/clang/Basic/riscv_vector.td did.
> > 
> > In fact, I don't have a strong opinion on which one is better
> > 
> > ps. current approach is similar to arm_sve.td design, maybe they know the some critical reason.
> I just find it really obfuscates things when we have all these magic character sequences.
Sorry, what do you mean?


================
Comment at: clang/include/clang/Basic/riscv_vector.td:66
+//      element type which is bool
+//   0: void type, ignores "t"
+//   z: size_t, ignores "t"
----------------
craig.topper wrote:
> jrtc27 wrote:
> > craig.topper wrote:
> > > jrtc27 wrote:
> > > > khchen wrote:
> > > > > jrtc27 wrote:
> > > > > > Then why aren't these just base types? We don't have to follow the brain-dead nature of printf.
> > > > > Basically builtin interface is instantiated by the "base type + LMUL" with type transformers. But in some intrinsic function we need a specific type regardless "base type + LMUL"
> > > > > ex. `vuint32m2_t vssrl_vx_u32m2_vl (vuint32m2_t op1, uint8_t op2, size_t vl);`
> > > > Then fix the way you define these? This is just bad design IMO.
> > > For each signature there is effectively a single key type that is a vector. The type transformer is a list of rules for how to derive all of the other operands from that one key type. Conceptually similar to LLVMScalarOrSameVectorWidth or LLVMHalfElementsVectorType in Intrinsics.td. Some types are fixed and don't vary by the key type. Like the size_t vl operand or a store intrinsic returning void.  There is no separate place to put a base type.
> > Oh I see, I hadn't appreciated that TypeRange got split up and each character was a whole separate intrinsic, I had misinterpreted it as it being a list of the types of arguments, i.e. an N-character string for an (N-1)-argument (plus return type) intrinsic that you could then use as a base and apply transformations too (e.g. "if" for a float-to-int intrinsic, with "vv" etc giving you a vectorised versions) and so was confused as to why the void/size_t/ptrdiff_t/uint8_t/bool-ness couldn't just be pushed into the TypeRange and the corresponding transforms left as "e". But, how _would_ you define vector float-to-int (and back) conversions with this scheme then?
> > 
> > On a related note, I feel one way to make this less obfuscated is change v/w/q/o to be v1/v2/v4/v8 (maybe with the 1 being optional, don't really care), and is also more extensible in future rather than ending up with yet more alphabet soup, though it does change the parsing from being a list of characters to a list of strings.
> I think these transforms are used for the float-to-int and int-to-float. 
> 
> //   I: given a vector type, compute the vector type with integer type
> //      elements of the same width
> //   F: given a vector type, compute the vector type with floating-point type
> //      elements of the same width
> 
> The float and integer types for conversion are always the same size or one lmul up or one lmul down. So combining I and F with v or w should cover it.
Yes, thanks for Craig's comments, I and U are used to implement conversion.

void/size_t/ptrdiff_t/uint8_t are not related to basic type so it's why they are no transformed from transforms left as `e`.

> On a related note, I feel one way to make this less obfuscated is change v/w/q/o to be v1/v2/v4/v8 (maybe with the 1 being optional, don't really care), and is also more extensible in future rather than ending up with yet more alphabet soup, though it does change the parsing from being a list of characters to a list of strings.

In the downstream we define a "complex" transformer which is not included in this patch. It  uses a string and encode additional information for some special type, like indexed operand of indexed load/store (its type is using EEW encoded in the instruction with EMUL=(EEW/SEW)*LMUL).
I have considered to use list of string, but personally I still prefer to use a list of characters because it's not often to use "complex" transformer and overall looking at the riscv_vector.td I feel current prototype is more readable and similar to intrinsic Builtins. 
Could we postpone the 'list of char' or 'list of string' decision in the future indexed load/store patch?



================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:259-260
+
+LMULType &LMULType::operator*=(unsigned RHS) {
+  this->Log2LMUL = this->Log2LMUL + RHS;
+  return *this;
----------------
craig.topper wrote:
> jrtc27 wrote:
> > That's not how multiplication works. This is exponentiation. Multiplication would be `Log2LMul + log2(RHS)`. Please don't abuse operators like this.
> This seems like it must be broken, but since we don't do widening or narrowing in this patch we didn't notice?
Yes, thanks for point out. In my original plan is fixing that in followup patches. 
I also add more bug fixes into this patch.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:870
+    PrevDef = Def.get();
+    OS << "case RISCV::BI__builtin_rvv_" << Def->getName() << ":\n";
+  }
----------------
jrtc27 wrote:
> Needs indentation?
I think switch case statement in `*_cg.inc` does not need indentation like `arm_cde_builtin_cg.inc` did.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95016