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

[Eric Christopher via llvm-commits]

On Tue, Apr 20, 2021 at 6:28 PM Philip Reames via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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.
>
>
Can you provide some examples of pretty much all of these? It sounds pretty
reasonable, but I don't think the API changes are visualizing for me at the
moment.


> > 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.
>
>
I mean, I guess if all you wanted was "I need someone to LGTM" then it's
fine, but that's not what it looked like as a proposal for discussion.


> If you think this is rushed and needs further discussion, I'm happy to
> revert while that discussion happens.
>
>
Feels a little rushed. Had less than 4 hours to even think about getting to
it.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210420/67ee3f60/attachment.html>