This is an archive of the discontinued LLVM Phabricator instance.

Don't invoke getName() from Function::isIntrinsic().
ClosedPublic

Authored by jlebar on Jul 6 2016, 1:12 PM.

Details

Summary

getName() involves a hashtable lookup, so is expensive given how
frequently isIntrinsic() is called. (In particular, many users cast to
IntrinsicInstr or one of its subclasses before calling
getIntrinsicID().)

This has an incidental functional change: Before, isIntrinsic() would
return true for any function whose name started with "llvm.", even if it
wasn't properly an intrinsic. The new behavior seems more correct to
me, because it's strange to say that isIntrinsic() is true, but
getIntrinsicId() returns "not an intrinsic".

Some callers want the old behavior -- they want to know whether the
caller is a recognized intrinsic, or might be one in some other version
of LLVM. For them, we added Function::hasIntrinsicLikeName(), which
checks whether the name starts with "llvm.".

This change is good for a 1.5% e2e speedup compiling a large Eigen
benchmark.

Diff Detail

Event Timeline

jlebar updated this revision to Diff 62949.Jul 6 2016, 1:12 PM
jlebar retitled this revision from to Don't invoke getName() from Function::isIntrinsic()..
jlebar updated this object.
jlebar added a subscriber: llvm-commits.
vsk added subscribers: escha, vsk.Jul 6 2016, 4:06 PM

I think @escha has tried something like this in the past, and would be a good reviewer.

Does having an intrinsic-like name carry any special meaning in llvm IR? I haven't really touched this stuff, but ISTM that behaving differently when you see an intrinsic-like name is suspicious.

include/llvm/IR/Function.h
141

Just assert this?

lib/IR/AsmWriter.cpp
3379

This chain of "if's" looks pretty familiar. Do you think it's worth pulling into a helper, e.g "MapOverIntrinsicMDValues" (in a separate commit)?

jlebar marked 2 inline comments as done.Jul 6 2016, 4:14 PM

Does having an intrinsic-like name carry any special meaning in llvm IR? I haven't really touched this stuff, but ISTM that behaving differently when you see an intrinsic-like name is suspicious.

Names starting with "llvm." are reserved for llvm's internal use, past, present, and future. So checking in the verifier that you don't take the address of any function named llvm. seems reasonable to me. The changes in AsmWriter are a bit more suspect: We're saying that you're allowed to attach metadata to any function named "llvm.". This seems pretty harmless, and is in fact useful if you wanted to emit IR for (say) a future version of llvm.

The changes to WinEHPrepare I'm less confident in. But I had a great deal of difficulty making the WinEH tests work with "real" intrinsics, and it's a conservative change, so I think it's OK (though certainly not ideal).

include/llvm/IR/Function.h
141

I was trying not to pull in Intrinsics.h -- otherwise I'd just return Intrinsic::not_intrinsic.

lib/IR/AsmWriter.cpp
3379

It might be worth someone doing that cleanup, yes. :)

jlebar updated this revision to Diff 65822.Jul 27 2016, 3:38 PM
jlebar marked 4 inline comments as done.

Apply Justin Bogner's suggestions.

In particular, the suggested change to Function::recalculateIntrinsicID seems
to have no performance impact.

This revision was automatically updated to reflect the committed changes.