[PATCH] D93534: [VP] Improve the VP intrinsic unittests

[Simon Moll via Phabricator via llvm-commits]

simoll added inline comments.


================
Comment at: llvm/unittests/IR/VPIntrinsicTest.cpp:67
+  ScopeOPC = None;
+#include "llvm/IR/VPIntrinsics.def"
+
----------------
kaz7 wrote:
> 1. In this unittest, test cases are generated by either from a result of `CreateVPDeclarationModule` call or through macros in `llvm/IR/VPIntrinsics.def`.  Isn't it possible to use one consistent way?
> 2. Generating unit tests directly through macros in `llvm/IR/VPIntrinsics.def` requires some techniques, but it makes review difficult.  For example, isn't it possible to generate a table first using macros, then test values in the table in unit tests, to make test system simple?
> 
> 
> 1. In this unittest, test cases are generated by either from a result of `CreateVPDeclarationModule` call or through macros in `llvm/IR/VPIntrinsics.def`.  Isn't it possible to use one consistent way?

These tests test different things. For example, this specific test function tests the wellformedness of the `VPIntrinsics.def` file.
The other tests check the the properties defined in the `VPIntrinsics.def` file actually apply to intrinsic declarations.
Really, i'd prefer to test the `include/llvm/IR/VPIntrinsics.def` directly against `include/llvm/IR/Intrinsics.td`. It's difficult to generate intrinsics declarations just from an intrinsic id (also we'd need to parse the `Intrinsics.td` file to figure out which vp intrinsics are there at all). There will be a builder class (`VPBuilder`) in later patches that will make it much easier to do that. Until then, i figured it is better to just build a test module with intrinsics declarations.

> 2. Generating unit tests directly through macros in `llvm/IR/VPIntrinsics.def` requires some techniques, but it makes review difficult.  For example, isn't it possible to generate a table first using macros, then test values in the table in unit tests, to make test system simple? 

Most functions in `VPIntrinsic` are defined with these macros. I see little value in testing the macros against themselves beyond internal consistency, for example, as the round tripping tests do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93534