[LLVMdev] getIntrinsicID() optimization, mark 2

[Nicolas Capens]

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
>
>