[VP] Improve the VP intrinsic unittests

[simoll]

Summary

Test that all VP intrinsics are tested.
Test intrinsic id -> opcode -> intrinsic id round tripping.
Test property scopes in the include/llvm/IR/VPIntrinsics.def file.

[simoll]

Added VPModuleComplete - test that all registered VP intrinsics are declared in the testing module.

[frasercrmck]

llvm/unittests/IR/VPIntrinsicTest.cpp

210

Nit but OPC seems inconsistent with OC above. In general, the all-caps variable names looks a bit out of place to me. I'd suggest Opc.

[kaz7]

This is what I feel when I see this test. Just a thought. Not a requirement. But, I think it's possible to improve this test in that way.

llvm/unittests/IR/VPIntrinsicTest.cpp

67

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?

[simoll]

llvm/unittests/IR/VPIntrinsicTest.cpp

67

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.

[simoll]

NFC. Use OC variable name consistently.

[simoll]

Can we accept this patch? In the end, it is a strict improvement over the existing one.. and it is "just" a unittest..

[frasercrmck]

Everything else here looks good to me (though I'm not officially a reviewer so take that with some amount of salt)

llvm/unittests/IR/VPIntrinsicTest.cpp

213

I know this wasn't introduced by this patch, but why Call and not something like Optional<unsigned> None? I just had to go and look up how GetFunctionalOpcodeForVP is implemented to see if this is a special case. It's also not documented by the comments above that method's declaration.

[simoll]

In D93534#2732969, @frasercrmck wrote:

Everything else here looks good to me (though I'm not officially a reviewer so take that with some amount of salt)

AFAIU everybody who has a stake in this is at least ok with this patch and waiting for somebody else to step up. Thanks for being that guy :) I'll let this sit until D78203 is recomitted to not have too much stuff in flight.

llvm/unittests/IR/VPIntrinsicTest.cpp

213

The idea here was that if there is no matching functional opcode then it is still a call of an intrinsic function (I had generalized pattern matching in mind D92086). But i guess that's just confusing and the function could just return a plain None.. noted for a followup patch.

llvm/unittests/IR/VPIntrinsicTest.cpp

 //===- VPIntrinsicTest.cpp - VPIntrinsic unit tests ---------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/AsmParser/Parser.h"
+#include "llvm/CodeGen/ISDOpcodes.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Verifier.h"
 #include "llvm/Support/SourceMgr.h"
 #include "gtest/gtest.h"
+#include <sstream>
 
 using namespace llvm;
 
 namespace {
 
 class VPIntrinsicTest : public testing::Test {
 protected:
   LLVMContext Context;
 
   VPIntrinsicTest() : Context() {}
 
   LLVMContext C;
   SMDiagnostic Err;
 
   std::unique_ptr<Module> CreateVPDeclarationModule() {
-      return parseAssemblyString(
-" declare <8 x i32> @llvm.vp.add.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32) "
-" declare <8 x i32> @llvm.vp.sub.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32) "
-" declare <8 x i32> @llvm.vp.mul.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32) "
-" declare <8 x i32> @llvm.vp.sdiv.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32) "
-" declare <8 x i32> @llvm.vp.srem.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32) "
-" declare <8 x i32> @llvm.vp.udiv.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32) "
-" declare <8 x i32> @llvm.vp.urem.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32) "
-" declare <8 x i32> @llvm.vp.and.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32) "
-" declare <8 x i32> @llvm.vp.xor.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32) "
-" declare <8 x i32> @llvm.vp.or.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32) "
-" declare <8 x i32> @llvm.vp.ashr.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32)  "
-" declare <8 x i32> @llvm.vp.lshr.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32)  "
-" declare <8 x i32> @llvm.vp.shl.v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32) ",
-          Err, C);
+    const char *BinaryIntOpcodes[] = {"add",  "sub",  "mul", "sdiv", "srem",
+                                      "udiv", "urem", "and", "xor",  "or",
+                                      "ashr", "lshr", "shl"};
+    std::stringstream Str;
+    for (const char *BinaryIntOpcode : BinaryIntOpcodes)
+      Str << " declare <8 x i32> @llvm.vp." << BinaryIntOpcode
+          << ".v8i32(<8 x i32>, <8 x i32>, <8 x i1>, i32) ";
+
+    return parseAssemblyString(Str.str(), Err, C);
   }
 };
 
+/// Check that the property scopes include/llvm/IR/VPIntrinsics.def are closed.
+TEST_F(VPIntrinsicTest, VPIntrinsicsDefScopes) {
+  Optional<Intrinsic::ID> ScopeVPID;
+#define BEGIN_REGISTER_VP_INTRINSIC(VPID, ...)                                 \
+  ASSERT_FALSE(ScopeVPID.hasValue());                                          \
+  ScopeVPID = Intrinsic::VPID;
+#define END_REGISTER_VP_INTRINSIC(VPID)                                        \
+  ASSERT_TRUE(ScopeVPID.hasValue());                                           \
+  ASSERT_EQ(ScopeVPID.getValue(), Intrinsic::VPID);                            \
+  ScopeVPID = None;
+
+  Optional<ISD::NodeType> ScopeOPC;
+#define BEGIN_REGISTER_VP_SDNODE(SDOPC, ...)                                   \
+  ASSERT_FALSE(ScopeOPC.hasValue());                                           \
+  ScopeOPC = ISD::SDOPC;
+#define END_REGISTER_VP_SDNODE(SDOPC)                                          \
+  ASSERT_TRUE(ScopeOPC.hasValue());                                            \
+  ASSERT_EQ(ScopeOPC.getValue(), ISD::SDOPC);                                  \
+  ScopeOPC = None;
+#include "llvm/IR/VPIntrinsics.def"
+
+  ASSERT_FALSE(ScopeVPID.hasValue());
+  ASSERT_FALSE(ScopeOPC.hasValue());
+}
+
+/// Check that every VP intrinsic in the test module is recognized as a VP
+/// intrinsic.
+TEST_F(VPIntrinsicTest, VPModuleComplete) {
+  std::unique_ptr<Module> M = CreateVPDeclarationModule();
+  assert(M);
+
+  // Check that all @llvm.vp.* functions in the module are recognized vp
+  // intrinsics.
+  std::set<Intrinsic::ID> SeenIDs;
+  for (const auto &VPDecl : *M) {
+    ASSERT_TRUE(VPDecl.isIntrinsic());
+    ASSERT_TRUE(VPIntrinsic::IsVPIntrinsic(VPDecl.getIntrinsicID()));
+    SeenIDs.insert(VPDecl.getIntrinsicID());
+  }
+
+  // Check that every registered VP intrinsic has an instance in the test
+  // module.
+#define BEGIN_REGISTER_VP_INTRINSIC(VPID, ...)                                 \
+  ASSERT_TRUE(SeenIDs.count(Intrinsic::VPID));
+#include "llvm/IR/VPIntrinsics.def"
+}
+
 /// Check that VPIntrinsic:canIgnoreVectorLengthParam() returns true
 /// if the vector length parameter does not mask off any lanes.
 TEST_F(VPIntrinsicTest, CanIgnoreVectorLength) {
   LLVMContext C;
   SMDiagnostic Err;
 
   std::unique_ptr<Module> M =
       parseAssemblyString(
if (RoundTripOC == Instruction::Call)
       continue;
 
     ASSERT_EQ(RoundTripOC, OC);
     ++FullTripCounts;
   }
   ASSERT_NE(FullTripCounts, 0u);
 }
 
+/// Check that going from VP intrinsic to Opcode and back results in the same
+/// intrinsic id.
+TEST_F(VPIntrinsicTest, IntrinsicIDRoundTrip) {
+  std::unique_ptr<Module> M = CreateVPDeclarationModule();
+  assert(M);
+
+  unsigned FullTripCounts = 0;
+  for (const auto &VPDecl : *M) {
+    auto VPID = VPDecl.getIntrinsicID();
+    unsigned OC = VPIntrinsic::GetFunctionalOpcodeForVP(VPID);
+
+    // no equivalent Opcode available
+    if (OC == Instruction::Call)
+      continue;
+
+    Intrinsic::ID RoundTripVPID = VPIntrinsic::GetForOpcode(OC);
+
+    ASSERT_EQ(RoundTripVPID, VPID);
+    ++FullTripCounts;
+  }
+  ASSERT_NE(FullTripCounts, 0u);
+}
+
 } // end anonymous namespace