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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Added VPModuleComplete - test that all registered VP intrinsics are declared in the testing module.
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. |
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 |
|
llvm/unittests/IR/VPIntrinsicTest.cpp | ||
---|---|---|
67 |
These tests test different things. For example, this specific test function tests the wellformedness of the VPIntrinsics.def file.
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. |
Can we accept this patch? In the end, it is a strict improvement over the existing one.. and it is "just" a unittest..
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. |
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. |