[PATCH] D99976: Allow invokable sub-classes of IntrinsicInst

Tue Apr 20 15:28:55 PDT 2021

reames added a comment.

In D99976#2703175 <https://reviews.llvm.org/D99976#2703175>, @echristo wrote:

> I guess my question is "why? what does this make easier/harder/etc?"

A couple of pieces here.

1. We no longer have the chance of a nasty false negative bug.  Before, we could fail to handle some intrinsics because they would not match IntrinsicInst.
2. As listed in the potential cleanups in the commit, we have multiple places in code which handled intrinsics generically, but then had to add extra casing for the invokable ones.  We'll be able to clean that up.
3. For intrinsics which really are calls - which, admittedly, is many - this does complicate the pattern matching as you have to both match the intrinsicinst, and then cast to CallInst for interfaces which expect calls.  (A quick survey shows many of those interfaces could reasonably take CallBase, but not all.)  We could explore having two base classes - one for all intrinsics and one for call only intrinsics - not quite sure if the multiple inheritance works out there.  It didn't seem worthwhile to justify exploration to me, if you disagree, let me know.

> I see you added a bunch of people and then committed, but I'd still like to hear the answers.

Er, did you intentionally leave out the "waited for LGTM" part?   Your wording seems to imply bad faith which I don't think is called for.

If you think this is rushed and needs further discussion, I'm happy to revert while that discussion happens.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99976