[PATCH] D99151: [RISCV][Clang] Add RVV vleff intrinsic functions.

[Zakk Chen via Phabricator via cfe-commits]

khchen added inline comments.


================
Comment at: clang/test/CodeGen/RISCV/rvv-intrinsics/vle16ff.c:9
+// RUN:   -target-feature +experimental-zfh -target-feature +m -fallow-half-arguments-and-returns -Werror -Wall -S -o - %s >/dev/null 2>%t
+// RUN: FileCheck --check-prefix=ASM --allow-empty %s <%t
+
----------------
jrtc27 wrote:
> jrtc27 wrote:
> > craig.topper wrote:
> > > jrtc27 wrote:
> > > > khchen wrote:
> > > > > nit: skip the temporary file and remove +experimental-zfh and -fallow-half-arguments-and-returns.
> > > > Can we please stop putting ASM tests in Clang, and if they really are needed, splitting them out into their own files separate from the IR tests?
> > > For more background, this approach was copied from AArch64 SVE where they said this was beneficial to catch warnings from TypeSize's implicit conversion to uint64_t.
> > You can have the ASM tests in a separate .test file though so you can still run the IR tests without the backend if there's value in having the end-to-end tests
> (well s/tests/RUN lines/)
IMO, this is also useful to catch mismatch in generated IR intrinsic with backend because we have `llvm_any_ty` type in intrinsic IR . In the downstream we met the same bug several time which generated intrinsic from builtin can not be selected in backend, and it 
was passed the builtin->IR test because we didn't have ASM test.

Do you mean we need to add the IR tests which are generated by clang builtin?
Currently we have the IR tests but they are generated by a script, do you mean we need to overwrite them? 
BTW, they are a little different to IR generated by builtin, for example:
https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/RISCV/rvv/vadd-rv32.ll
IR intrinsic is `llvm.riscv.vadd.nxv1i8.nxv1i8` in RV32 and RV64
https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGen/RISCV/rvv-intrinsics/vadd.c
IR intrinsic is `llvm.riscv.vadd.nxv1i8.nxv1i8.i32` in RV32, `llvm.riscv.vadd.nxv1i8.nxv1i8.i64`  in RV64.

I don't like to add too much IR tests, so I'm OK to remove ASM test. But in the future, reviewer maybe need to verify the generated IR can generate asm without any error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99151