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

[Zakk Chen via Phabricator via llvm-commits]

khchen added a comment.

Really thanks for @jrtc27 and @craig.topper 's review suggestions.
Before I upload the patch, I would want to discuss about the test generator, 
because if we don't want to upstream it, I don't need to fix some issues which are addressed by reivewers.

@asb @jrtc27 @craig.topper @evandro @HsiangKai : 
What's your opinion about the intrinsic test generator, should we really need to upstream it?
I'm just afraid when any community people want to contribute this part, they need to rewrite it again by themselves.
In the future we can remove test generator when all intrinsic are upstreamed.

However, I'm also ok to remove test generator if more people prefer on removing it.



================
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:
> 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.


================
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"
----------------
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);`


================
Comment at: clang/utils/TestUtils/gen-riscv-v-tests.sh:22
+# op_list have marco name used in riscv_vector.td
+  local op_list="VADD VFADD"
+  local option="$1"
----------------
craig.topper wrote:
> It feels a little weird that this list is in the script and not derived from the td file automatically somehow. Ideally we wouldn't have to update the script every time a new set of intrinsics is added.
In tablegen interface `EmitRVVTest(RecordKeeper &Records, raw_ostream &OS)` can only emit one file. Currently I only found this stupid way to control `clang-tblgen` to generate one op in different files. 
Do you think it is acceptable to have a huge test file?
or maybe we can generate the same category op (ex. integer ops, floating ops) in the same file and predefined the all category first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95016