This is an archive of the discontinued LLVM Phabricator instance.

[VP] Improve the VP intrinsic unittests
ClosedPublic

Authored by simoll on Dec 18 2020, 4:34 AM.

Details

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.

Diff Detail

Event Timeline

simoll requested review of this revision.Dec 18 2020, 4:34 AM
simoll created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 18 2020, 4:34 AM
simoll updated this revision to Diff 312758.Dec 18 2020, 4:42 AM

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

khchen added a subscriber: khchen.Dec 19 2020, 6:31 AM
frasercrmck added inline comments.Dec 21 2020, 2:58 AM
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 added a comment.Dec 21 2020, 5:40 AM

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 added inline comments.Dec 21 2020, 6:25 AM
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.

  1. 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 updated this revision to Diff 314338.Jan 4 2021, 2:15 AM
simoll marked an inline comment as done.

NFC. Use OC variable name consistently.

simoll added a comment.Jan 8 2021, 2:56 AM

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

frasercrmck accepted this revision.May 3 2021, 1:56 AM

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.

This revision is now accepted and ready to land.May 3 2021, 1:56 AM
simoll added a comment.May 3 2021, 6:24 AM

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.

This revision was automatically updated to reflect the committed changes.