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

[Sean Silva via llvm-commits]

On Tue, Oct 6, 2015 at 10:55 AM, Pete Cooper <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> 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.

-- 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
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > 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/49a62e57/attachment.html>