Page MenuHomePhabricator

Allow invokable sub-classes of IntrinsicInst
AcceptedPublic

Authored by reames on Tue, Apr 6, 10:40 AM.

Details

Summary

It used to be that all of our intrinsics were call instructions, but over time, we've added more and more invokable intrinsics. According to the verifier, we're up to 8 right now. As IntrinsicInst is a sub-class of CallInst, this puts us in an awkward spot where the idiomatic means to check for intrinsic has a false negative if the intrinsic is invoked.

This change switches IntrinsicInst from being a sub-class of CallInst to being a subclass of CallBase. This allows invoked intrinsics to be instances of IntrinsicInst, at the cost of requiring a few more casts to CallInst in places where the intrinsic really is known to be a call, not an invoke.

This is admittedly a debatable stylistic issue. To me, allowing invokeable intrinsics seems cleaner, but I also spent a lot of my time dealing with those intrinsics. Do others agree?

At the moment, this patch only builds on x86. I'm sure there will be analogous changes in the other targets. I'll do them if folks are okay with the overall direction.

After this lands (if it does), planned cleanups:

  • Make GCStatepointInst a IntrinsicInst subclass.
  • Merge intrinsic handling in InstCombine and use idiomatic visitIntrinsicInst entry point for InstVisitor.
  • Do the same in SelectionDAG.
  • Do the same in FastISEL.

Diff Detail

Event Timeline

reames created this revision.Tue, Apr 6, 10:40 AM
reames requested review of this revision.Tue, Apr 6, 10:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Apr 6, 10:40 AM
kpn added a subscriber: kpn.Tue, Apr 6, 1:19 PM
reames updated this revision to Diff 335652.Tue, Apr 6, 1:48 PM
reames retitled this revision from [WIP] Allow invokable sub-classes of IntrinsicInst to Allow invokable sub-classes of IntrinsicInst.
reames edited the summary of this revision. (Show Details)

Rebase over cleanups - realized several of the things I was going to do to reduce diffs were generally useful code improvements so just went ahead and did them.

nikic accepted this revision.Tue, Apr 20, 11:19 AM

LGTM. The number of necessary casts seems to be pretty small, so I think this is a reasonable tradeoff.

llvm/include/llvm/IR/InstVisitor.h
313

non-CallBase

This revision is now accepted and ready to land.Tue, Apr 20, 11:19 AM
This revision was landed with ongoing or failed builds.Tue, Apr 20, 3:06 PM
This revision was automatically updated to reflect the committed changes.

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

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

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.

(For anyone following along, discussion has moved to email on review thread which phabricator isn't catching.)

reames reopened this revision.Tue, Apr 20, 3:48 PM
This revision is now accepted and ready to land.Tue, Apr 20, 3:48 PM
nikic added a comment.Wed, Apr 21, 1:47 PM

Interesting, I didn't expect that. Presumably this is because most places working with IntrinsicInst now have to perform two branches rather than one, and apparently it gets used a lot. Most likely this overestimates actual impact (assuming predicted branches), but still, better to avoid it if we can.

Could it make sense to have both IntrinsicBase and IntrinsicInst, similar to how we have CallBase and CallInst?