getIntrinsicID() optimization, mark 2

Any takers? This patch improves on the previous one by making getIntrinsicID() inline.

FastIntrinsicID-2.patch (2.54 KB)

It is possible to change the name of a Function with Value::setName,
so this patch _could_ cause incorrect answers. You should add a test
that calls setName("") to make sure this keeps working,
regardless of where this patch goes. You might be able to catch such
events in the ValueSymbolTable and update the intrinsic ID, but I
can't find an obvious place to put that code.

I also noticed that getIntrinsicID (implemented in Intrinsics.gen) is
a switch statement on the first letter of the intrinsic name plus a
long series of
    if (Len == 16 && !memcmp(Name, "llvm.alpha.umulh", 16)) return
    if (Len > 15 && !memcmp(Name, "llvm.annotation.", 16)) return
    if (Len > 20 && !memcmp(Name, "llvm.arm.neon.vabals.", 21)) return

There has to be a more efficient way to do this. If nothing else, the
first 6 characters of that memcmp are always equal, and for groups of
intrinsics like llvm.arm.neon.v*, a trie-like search would be better.
llvm/ADT/Trie.h doesn't quite do what this needs (no prefix
searching), and it looks like it should be changed to use StringRefs
instead of std::strings during the search, but fixing and using it
here might be the right thing to do.

Hi Jeffrey,

Please correct me if I'm wrong, but I believe a test that checks for the old
behavior would be pointless. I realize that my patch would return an
unmatching intrinsicID when the function's name changes, but the real
question we should ask is do we really want to be able to change the
intrinsicID by changing the function's name?

Also, the original Function constructor contains this code:

// Ensure intrinsics have the right parameter attributes.
if (unsigned IID = getIntrinsicID())

Note that if you hypothetically changed an intrinsic's name it's parameter
attributes could potentially become invalid...

It seems to me that intrinsic names were never intended to be changed in the
first place. That sounds perfectly reasonable to me, although it should
probably be documented somewhere. So unless there are tangible reasons why
intrinsics should actually be allowed to change name I believe my patch is



Sorry, I should have said that you should add a test checking the
correct behavior. I don't really have an opinion on which behavior is
correct--I just assumed you didn't realize the behavior would change.