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

[Philip Reames via llvm-commits]

On 4/20/21 3:32 PM, Eric Christopher wrote:
>
>
> On Tue, Apr 20, 2021 at 6:28 PM Philip Reames via Phabricator 
> <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
>
>     reames added a comment.
>
>     In D99976#2703175 <https://reviews.llvm.org/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.

Er, I sorta feel I already have?  1 is hypothetical, but easy for any of 
the invokable ones (see list in Verifier).  2 requires me to write a 
patch, but should be obvious from inspecting the code in e.g. 
instcombine, and 3 is covered by the patch you're responding to.  I'm 
not trying to be difficult here, but I'm honestly not sure what more you 
want?

>     > 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.
(combined response with next point)
>
>     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.

Reverted in 4824d876f0 while this discussion happens.

I think you're misrepresenting this a bit.  I put out a proposal, that 
did include a question to sanity check with others.  I got no response 
for two weeks.  I directly added several parties known to be interested 
in big picture API questions.  I got a single response which seemed to 
endorse said proposal with an explicit LGTM.  You raised concerns, I 
responded promptly.  This seems all around reasonable and normal 
practice to me.

Philip


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210420/f9c46ef3/attachment-0001.html>