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

[Pete Cooper via llvm-commits]

> On Oct 6, 2015, at 4:33 PM, Sean Silva <chisophugis at gmail.com> wrote:
> 
> 
> 
> On Tue, Oct 6, 2015 at 10:55 AM, Pete Cooper <peter_cooper at apple.com <mailto:peter_cooper at apple.com>> wrote:
> 
> > On Oct 5, 2015, at 4:57 PM, Sean Silva via llvm-commits <llvm-commits at lists.llvm.org <mailto: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.
> 
> Makes sense. I was assuming that getName was just following a StringRef inside Function (or one of its base classes).
>  
> 
> 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.
> 
> But is it actually "allowed" for us to even get into a "not known, but is an intrinsic" state? That seems like something that we would need to autoupgrade (but then it would become known). The verifier could check this to prevent us from getting into an inconsistent state.
I can imagine out-of-tree targets wanting to be able to pass their IR which contains their intrinsics to ToT tools and have it be ok with it.  So long as passes optimize calls to unknown intrinsics using the attributes on the call to choose what is safe, then I imagine it should work ok.  Of course that’s assuming this is a use case we want to support.  

Other tools like llvm-nm shouldn’t care about intrinsics, although I guess they shouldn’t list an intrinsic in the list of symbols as its assumed someone will remove the intrinsic call during compilation, even if that someone is just isel.

BTW, I’m not arguing that we should support unknown intrinsics or not (honestly, I just have no idea what the right choice is), just giving one interpretation of what the current behavior is.

Cheers,
Pete
> 
> -- Sean Silva
>  
> 
> 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 <http://reviews.llvm.org/D13427>
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151006/2b3e2c9f/attachment.html>