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

Mon May 3 06:24:23 PDT 2021

simoll added a comment.

In D93534#2732969 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D78203> is recomitted to not have too much stuff in flight.

================
Comment at: llvm/unittests/IR/VPIntrinsicTest.cpp:213
+    // no equivalent Opcode available
+    if (OC == Instruction::Call)
+      continue;
----------------
frasercrmck wrote:
> 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.
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93534