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

[Eric Christopher via llvm-commits]

On Tue, Apr 20, 2021 at 6:48 PM Philip Reames <listmail at philipreames.com>
wrote:

>
> 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> 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.
>
> 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 was thinking a couple of examples of "here are the kinds of API changes I
expect to be able to make and this will make things safer and this is what
the code will look like at the end".

i.e.

Instead of writing:

blah

I'll write

baz and foo->bar()

in the future, but that solves the long term maintainability of X because
blah could have a subtle bug.

That make sense?


>
>
>> > 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.
>
That's fine, I would have just expected for something that seemed like a
proposal to wait at least 24 hours after making people aware of it :) No
harm done though!

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