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