[LLVMdev] getIntrinsicID() optimization, mark 2

[Jeffrey Yasskin]

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.

On Sat, Oct 17, 2009 at 4:08 PM, Nicolas Capens <nicolas at capens.net> wrote:
> 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())
>    setAttributes(Intrinsic::getAttributes(Intrinsic::ID(IID)));
>
> 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
> valid.
>
> Cheers,
>
> Nicolas
>
>
> -----Original Message-----
> From: Jeffrey Yasskin [mailto:jyasskin at google.com]
> Sent: Saturday, 17 October, 2009 19:03
> To: Nicolas Capens
> Cc: LLVM Developers Mailing List
> Subject: Re: [LLVMdev] getIntrinsicID() optimization, mark 2
>
> 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("intrinsic.name") 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
> Intrinsic::alpha_umulh;
>    if (Len > 15 && !memcmp(Name, "llvm.annotation.", 16)) return
> Intrinsic::annotation;
>    if (Len > 20 && !memcmp(Name, "llvm.arm.neon.vabals.", 21)) return
> Intrinsic::arm_neon_vabals;
> ...
>
> 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.
>
> On Sat, Oct 17, 2009 at 3:48 AM, Nicolas Capens <nicolas at capens.net> wrote:
>> Any takers? This patch improves on the previous one by making
>> getIntrinsicID() inline.
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>
>>
>
>