[PATCH] D22949: Speed up Function::isIntrinsic() by adding a bit to GlobalValue. NFC

[Justin Bogner via llvm-commits]

Reid Kleckner <rnk at google.com> writes:
> rnk added a subscriber: beanz.
> rnk added a comment.
>
> In https://reviews.llvm.org/D22949#518862, @jlebar wrote:
>
>> Justin Bogner wrote in an email to the list:
>>
>> > I'm a bit less comfortable with this change than I was with the other
>>
>> >  one - it's more of a straight performance hack this way, whereas the
>>
>> >  other way fit better in a future where target-specific intrinsics are
>>
>> >  more of a first class idea.
>
> Do you think we should move more towards LLVM having a single enum of
> all target intrinsics, or more towards a place where LLVM has target
> neutral intrinsics, and then there is a separate target-specific
> intrinsic ID space that mid-level optimizers can't know about?

Today we mostly have a single enum of all target intrinsics, and a very
old attempt at splitting things into separate target specific intrinsics
that didn't quite work out. I laid out some ideas about how to fix all
of this in an RFC a while back, but TLDR: I'd like to get all of the
intrinsics done the same way (the big table), stop using target specific
stuff in generic code, and then split the table out into a two-level
look up of generic and target specific.

jlebar's original change fit better with that model and would evolve
naturally with these changes. This current patch doesn't really hurt my
plans, but it will essentially get reverted and turned into something
more like the original change at some point in the process.

> This change actually seems like a step towards that world, which the
> AMDGPU backend is already half-in: all intrinsics are named "llvm.*"
> and many of them are not included in Intrinsic::ID.
>
> Maybe in the long run we should move all target intrinsics out of
> include/llvm/IR/Intrinsics*.td, and down into lib/Target. We'd use
> TargetIntrinsicInfo to look up the ID. Then we can go back to the old,
> non-bitfield solution. It also makes it easier for us to compile out
> the intrinsic name to ID tables for disabled targets, which I think
> @beanz wanted to do. I don't think IR can talk to Target, so we
> probably can't do this the obvious way.

This needs to work in such a way that the mid level optimizer can still
treat the target specific intrinsics and intrinsics, and ideally be able
to look up some obvious things about them. Notably, the current way to
hide your intrinsics in a backend (which only amdgpu uses at all)
doesn't hook into the verifier at all and requires a bunch of
boilerplate/duplicate code from the generic logic.

In any case, yes, this is basically the end goal.