[PATCH] D13427: RFC: faster isa<IntrinsicInst> (bugged tests?)

[Pete Cooper via llvm-commits]

> On Oct 5, 2015, at 4:57 PM, Sean Silva via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> silvas added a subscriber: silvas.
> silvas added a comment.
> 
> We should optimize getName().startswith("llvm.") on x86 into loading the address and length of the name, a bounds check, a 4-byte integer comparison, a 1-byte integer comparison, and cmp+jmp for each comparison. Assuming the name is in cache (which is probably not the case?) and the branch prediction is good (which it probably is?), that is < 10 cycles on a modern x86. Even on ARM where we can't use unaligned loads, it should still be pretty darn fast.
FWIW, you’re probably right about the speed of checking if a string is "llvm.”.  I haven’t looked at the assembly generated, but its probably close enough to what you’ve said here.

I suspect the majority of the 50 cycles is coming from getName() itself, which looks up the Function* in a map to get its string.  

A map for names makes sense for Instruction and Constant which are likely to not have a name, but perhaps for GlobalValue and its subclasses, we should just store the name in the class itself.

For performance, seems like the easiest solution is to set a bit on a Function to say whether it ‘isIntrinsic()’.  That bit can be set in recalculateIntrinsicID() which is already called when the name changes.  We can even steal a bit from 'Intrinsic::ID IntID;’ if we want, or make that field ‘Optional<Intrinsic::ID>’ if we could teach Optional some tricks about the range of values and storing a tag bit inline with the data.

And then getIntrinsicID() is telling us whether what we have is a known intrinsic (not_intrinsic means ’not known’, but it is an intrinsic, just not one we know anything about), and the name is telling us whether it is an intrinsic, known or not.  Assuming folks are happy with those terms, we then just have to vet the 28 hits in clang/LLVM for isIntrinsic() and make sure they match.  I’m pretty sure getIntrinsicID() is already consistent, but then we could throw an assert in getIntrinsicID() for ‘assert(isIntrisnic()…)’ so make sure that no-one calls getIntrinsicID() without being sure its an intrinsic first.

Pete
> 
> What is actually taking ~50 cycles? How did you measure that?
> 
> (I'm not defending using a string comparison vs. not; just wondering why a comparison against a "short, constant string" is ending up so slow)
> 
> 
> Repository:
>  rL LLVM
> 
> http://reviews.llvm.org/D13427
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits