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

[Philip Reames via llvm-commits]

On 4/20/21 3:54 PM, Eric Christopher wrote:
>
>
> On Tue, Apr 20, 2021 at 6:48 PM Philip Reames 
> <listmail at philipreames.com <mailto: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 <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 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?

As an example, consider the current code structure from instcombine.

 From visitCallInst:

   IntrinsicInst *II = dyn_cast<IntrinsicInst>(&CI);
   if (!II) return visitCallBase(CI);
   ...
   Intrinsic::ID IID = II->getIntrinsicID();
   switch (IID) {
   case Intrinsic::objectsize:
   ... (sometimes uses dyn_cast<IntrinsicInst> for argument matching)

 From visitCallBase:

   // Handle intrinsics which can be used in both call and invoke context.
   switch (Call.getIntrinsicID()) {
   case Intrinsic::experimental_gc_statepoint: {
     GCStatepointInst &GCSP = *cast<GCStatepointInst>(&Call);
   .. etc
   }

Would become:

visitiIntrinsicInst
(using the InstVisitor dispatch point enabled by this change, and thus 
no separate explicit cast)

   Intrinsic::ID IID = II->getIntrinsicID();
   switch (IID) {
   case Intrinsic::objectsize:
   ... (sometimes uses dyn_cast<IntrinsicInst> for argument matching)
   case Intrinsic::experimental_gc_statepoint: {
     GCStatepointInst &GCSP = *cast<GCStatepointInst>(&Call);
   .. etc
   }

That seems cleaner to me.  We get similar cleanup in SelectionDAG.

I do want to acknowledge that we can get a good amount of that 
unification by just using CallBase in place of IntrinsicInst, but well, 
at that point what's the point of having intrinsic inst?


An example where this makes code slightly worse (from FastISel in 
original patch)

   case Intrinsic::xray_customevent:
     return selectXRayCustomEvent(cast<CallInst>(II));

The casts to CallInst are new.  This is a call where having a 
distinction between InstrinicBase and IntrinsicInst (as suggested in 
another response) might help.

I'll note in passing that several cases in the original patch where we 
cast to CallInst, we could instead change the interface to return 
IntrinsicInst or CallBase and everything just works.  I was trying to 
minimize diff size, which makes the code impact look slightly worse than 
it'd end up being.

>>         > 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/20210423/900a5b47/attachment.html>