Page MenuHomePhabricator

[LoopVectorize] Guard verifyFunction with EXPENSIVE_CHECKS macro
Needs ReviewPublic

Authored by tislam on Tue, Jan 12, 6:36 PM.

Details

Summary

We have found that the calls to verifyFunction can take up to 66% of the time spent in llvm::LoopVectorizePass::processLoop. For some source files, verifyFunction took around 10% of the total compile time.

This patch guards the calls to verifyFunction with the EXPENSIVE_CHECKS macro.

Diff Detail

Event Timeline

tislam created this revision.Tue, Jan 12, 6:36 PM
tislam requested review of this revision.Tue, Jan 12, 6:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jan 12, 6:36 PM
fhahn added a subscriber: fhahn.Wed, Jan 13, 2:00 AM

We have found that the calls to verifyFunction can take up to 66% of the time spent in llvm::LoopVectorizePass::processLoop. For some source files, verifyFunction took around 10% of the total compile time.

See also https://bugs.llvm.org/show_bug.cgi?id=47712. I'm curious, where is the time spent in the verifier? PR47712 mentions a regression in the verifier where the majority of time is spent in visitMDNode. Is this a similar issue?

In general, I think the function verification has been very valuable in terms of surfacing bugs and I would be very reluctant to move it behind expensive checks, especially since we had this check for a long time. As mentioned in PR47712, I think verifyFunction should be O(# instructions in function) and not behind EXPENSIVE_CHECKS.

tislam added a comment.EditedWed, Jan 13, 6:42 AM

We have found that the calls to verifyFunction can take up to 66% of the time spent in llvm::LoopVectorizePass::processLoop. For some source files, verifyFunction took around 10% of the total compile time.

See also https://bugs.llvm.org/show_bug.cgi?id=47712. I'm curious, where is the time spent in the verifier? PR47712 mentions a regression in the verifier where the majority of time is spent in visitMDNode. Is this a similar issue?

In general, I think the function verification has been very valuable in terms of surfacing bugs and I would be very reluctant to move it behind expensive checks, especially since we had this check for a long time. As mentioned in PR47712, I think verifyFunction should be O(# instructions in function) and not behind EXPENSIVE_CHECKS.

Thanks @fhahn for the information on PR47712.

The above stats are for cases where the instructions do not have any metadata (like debug or alias) in general. Here are some profile snapshots.

|      |             |     --3.96%--llvm::LoopVectorizePass::runImpl
|      |             |      |
|      |             |       --3.96%--llvm::LoopVectorizePass::processLoop
|      |             |        |
|      |             |        |--2.63%--llvm::verifyFunction
|      |             |        |    |
|      |             |        |     --2.54%--(anonymous namespace)::Verifier::verify
|      |             |        |      |
|      |             |        |      |--1.95%--llvm::InstVisitor<(anonymous namespace)::Verifier, void>::visit<llvm::ilist_iterator<llvm::ilist_detail::node_options<llvm::BasicBlock, true, false, void>, false, false> >
|      |             |        |      |
|      |             |        |       --0.54%--llvm::DomTreeBuilder::Calculate<llvm::DominatorTreeBase<llvm::BasicBlock, false> >
|      |             |        |
|      |             |         --1.17%--llvm::LoopVectorizationPlanner::executePlan

With our local branch, we saw the following.

|      |      |     --14.69%--llvm::LoopVectorizePass::runImpl
|      |      |      |
|      |      |       --14.69%--llvm::LoopVectorizePass::processLoop
|      |      |        |
|      |      |        |--9.98%--llvm::verifyFunction
|      |      |        |    |
|      |      |        |     --9.59%--(anonymous namespace)::Verifier::verify
|      |      |        |      |
|      |      |        |      |--2.49%--(anonymous namespace)::Verifier::visitInstruction
|      |      |        |      |    |
|      |      |        |      |     --1.28%--(anonymous namespace)::Verifier::visitInstruction
|      |      |        |      |
|      |      |        |      |--2.35%--llvm::InstVisitor<(anonymous namespace)::Verifier, void>::visit
|      |      |        |      |    |
|      |      |        |      |     --2.04%--(anonymous namespace)::Verifier::visitInstruction
|      |      |        |      |      |
|      |      |        |      |       --1.14%--(anonymous namespace)::Verifier::visitInstruction
|      |      |        |      |
|      |      |        |      |--2.12%--(anonymous namespace)::Verifier::verify
|      |      |        |      |
|      |      |        |       --2.06%--llvm::DomTreeBuilder::SemiNCAInfo<llvm::DominatorTreeBase<llvm::BasicBlock, false> >::CalculateFromScratch
|      |      |        |        |
|      |      |        |        |--0.89%--llvm::DomTreeBuilder::SemiNCAInfo<llvm::DominatorTreeBase<llvm::BasicBlock, false> >::runDFS<false, bool (*)(llvm::BasicBlock*, llvm::BasicBlock*)>
|      |      |        |        |
|      |      |        |        |--0.60%--llvm::DomTreeBuilder::SemiNCAInfo<llvm::DominatorTreeBase<llvm::BasicBlock, false> >::attachNewSubtree
|      |      |        |        |
|      |      |        |         --0.52%--llvm::DomTreeBuilder::SemiNCAInfo<llvm::DominatorTreeBase<llvm::BasicBlock, false> >::runSemiNCA
|      |      |        |
|      |      |         --4.22%--llvm::LoopVectorizationPlanner::executePlan

So are the compilation time issues reproducible with vanilla llvm, or only in your local branch?
If latter, shouldn't the fix (which isn't hiding the verification under) belong to the branch?

So are the compilation time issues reproducible with vanilla llvm, or only in your local branch?
If latter, shouldn't the fix (which isn't hiding the verification under) belong to the branch?

Hi @lebedev.ri, the first profile is with the vanilla llvm.

So are the compilation time issues reproducible with vanilla llvm, or only in your local branch?
If latter, shouldn't the fix (which isn't hiding the verification under) belong to the branch?

Hi @lebedev.ri, the first profile is with the vanilla llvm.

That is what the message said yes.

Thanks - if we can fix/limit verifyFunction itself instead of the callers, that seems like a better solution (and we would revert that change in SLP).

So are the compilation time issues reproducible with vanilla llvm, or only in your local branch?
If latter, shouldn't the fix (which isn't hiding the verification under) belong to the branch?

Hi @lebedev.ri, the first profile is with the vanilla llvm.

That is what the message said yes.

@tislam to make progress here, can you share a reproducer that shows the problematic compile time implications on vanilla llvm?

kkwli0 added a subscriber: kkwli0.Wed, Jan 13, 10:58 AM
tislam added a comment.EditedThu, Jan 14, 6:36 AM

Hi @lebedev.ri, I have uploaded an IR file that demonstrates the compile-time implications on vanilla llvm on our system. I used the following invocation.

opt  -passes=default\<O3\> -aa-pipeline=default -mcpu=pwr9 -o ./t.O3.bc ./t.ll