Differential D49337
Skip debuginfo intrinsic in markLiveBlocks. Authored by trentxintong on Jul 13 2018, 9:38 PM.
Details The optimizer is 10%+ slower with vs without debuginfo. I started checking where I compiled sqlite3.c with and without debug info from CTMark and compare the time difference. I use Xcode Instrument to find where time is spent. This brings about 20ms, out of ~20s.
Diff Detail
Event TimelineComment Actions There's nothing special about debug intrinsics here except that there are a lot of them. The problem, as far as I can tell, is that we're repeatedly using dyn_cast on each instruction and doing multiple redundant tests. Adding yet another redundant test will help having a lot of debug intrinsics makes things incrementally more expensive for all other kinds of intrinsics. Thus, this doesn't seem like the right way to fix this. Instead of testing for IntrinsicInst, and then CallInst (which is always true whenever the IntrinsicInst is true), and then we always test for StoreInst (but we don't use an 'else' so we always do this test even when the IntrinsicInst/CallInst is true (which will include debug intrinsics)). First, I'd try adding an 'else' when testing for a store. Also, get rid of the IntrinsicInst test, getIntrinsicID is a member of Function, so you can do dyn_cast Callee to Function, and then check for the intrinsic IDs. Comment Actions Hi @hfinkel. Thank you for taking a look at this code. I agree we should definitely "else" the StoreInst check and I have observed improvements in Xcode Instrument for doing so. And I should also move the DbgInfoIntrinsic check inside the IntrinsicInst check by comparing IntrinsicID. The problem with moving the IntrinsicInst checks inside the dyn_cast<CallInst> is that every intrinsic call will need to go through a dyn_cast<CallInst> check, a getCalledValue() and then a dyn_cast<Function>(CalledValue) check. If we have a lot of intrinsic which we do when compiling with debuginfo, we could end up being overall slower than keeping the IntrinsicInst check. When we compile without debuginfo, moving the intrinsic check inside the CallInst check will not necessarily bring us performance, as now every CallInst needs to checks for the existing cases in the if (dyn_cast<CallInst>)) and the Intrinsics (which require dyn_cast<Function>(CalledValue)). Right now I am seeing the version which we keep the IntrinsicInst check and adding the DbgInfoIntrinsic inside it to be the fastest on sqlite3.c with debuginfo. Comment Actions Address some of @hfinkel suggestions. This brings ~25ms now out of ~20s spent to compile sqlite3.c. Comment Actions I'm not sure that I understand what you're saying. "is that every intrinsic call will need to go through a dyn_cast<CallInst> check, a getCalledValue() and then a dyn_cast<Function>(CalledValue) check." - this is exactly what the IntrinsicInst dyn_cast does internally. When we cast to IntrinsicInst we call the following in IntrinsicInst: static bool classof(const CallInst *I) {
if (const Function *CF = I->getCalledFunction())
return CF->isIntrinsic();
return false;
}
static bool classof(const Value *V) {
return isa<CallInst>(V) && classof(cast<CallInst>(V));
}so we pay for the isa<CallInst>, which should be the same cost as the dyn_cast<CallInst>, and then call I->getCalledFunction(), which is implemented as a dyn_cast<Function> of getCalledValue(). When we do isa<DbgInfoIntrinsic>, we do all of that, and then also call getIntrinsicID and compare to one of the IDs for the debug info intrinsics. Thus, to be clear, what I'm saying is that if we're worried about duplicated work here, we shouldn't be using IntrinsicInst at all.
Comment Actions @hfinkel I get what you mean. Basically moving the IntrinsicInst check inside the call does not result us doing extra work when processing IntrinsicInst as we essentially do the same things in the dyn_cast<IntrinsicInst>. I will update the diff accordingly. |
You shouldn't need to check for the debugging intrinsics at all. We're skipping all intrinsics except for assume and experimental_guard.