[PATCH] D12267: Require Dominator Tree For SROA, improve compile-time

[Michael Zolotukhin via llvm-commits]

Hi Mehdi,

Could you please explain why does it save compile time in the end? Why does SROA work slower when DT isn’t available? While your logic to construct DT before SROA seems legit, could that be that there is another bug in SROA itself (and we’re just papering it over now)?

Thanks,
Michael

> On Aug 22, 2015, at 10:26 PM, Mehdi AMINI via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> joker.eph created this revision.
> joker.eph added a reviewer: chandlerc.
> joker.eph added a subscriber: llvm-commits.
> 
> TL-DR: SROA is followed by EarlyCSE which requires the DominatorTree.
> There is no reason not to require it up-front for SROA.
> 
> Some history is necessary to understand why we ended-up here.
> 
> r123437 switched the second (Legacy)SROA in the optimizer pipeline to
> use SSAUpdater in order to avoid recomputing the costly
> DominanceFrontier. The purpose was to speed-up the compile-time.
> 
> Later r123609 removed the need for the DominanceFrontier in
> (Legacy)SROA.
> 
> Right after, some cleanup was made in r123724 to remove any reference
> to the DominanceFrontier. SROA existed in two flavors: SROA_SSAUp and
> SROA_DT (the latter replacing SROA_DF).
> The second argument of `createScalarReplAggregatesPass` was renamed
> from `UseDomFrontier` to `UseDomTree`.
> I believe this is were a mistake was made. The pipeline was not
> updated and the call site was still:
>    PM->add(createScalarReplAggregatesPass(-1, false));
> 
> At that time, SROA was immediately followed in the pipeline by
> EarlyCSE which required alread the DominatorTree. Not requiring
> the DominatorTree in SROA didn't save anything, but unfortunately
> it was lost at this point.
> 
> When the new SROA Pass was introduced in r163965, I believe the goal
> was to have an exact replacement of the existing SROA, this bug
> slipped through.
> 
> You can see currently:
> 
> $ echo "" | clang -x c++  -O3 -c - -mllvm -debug-pass=Structure
> ...
> ...
>      FunctionPass Manager
>        SROA
>        Dominator Tree Construction
>        Early CSE
> 
> After this patch:
> 
> $ echo "" | clang -x c++  -O3 -c - -mllvm -debug-pass=Structure
> ...
> ...
>      FunctionPass Manager
>        Dominator Tree Construction
>        SROA
>        Early CSE
> 
> This improves the compile time from 88s to 23s for PR17855.
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__llvm.org_bugs_show-5Fbug.cgi-3Fid-3D17855&d=BQIFaQ&c=eEvniauFctOgLOKGJOplqw&r=ygVmcuuQ1MUhRUoJm-IgPtgjmvM0byfjlHDg99vufEI&m=c4SYwnFJ6SDBptqMe1VTbbQusWp_1xoHHj7L0Jc-smc&s=ZpzLWG1SeKqKaDpe0Ax8lnOIgxj3pYR1bQds6tUIAqU&e= 
> 
> https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D12267&d=BQIFaQ&c=eEvniauFctOgLOKGJOplqw&r=ygVmcuuQ1MUhRUoJm-IgPtgjmvM0byfjlHDg99vufEI&m=c4SYwnFJ6SDBptqMe1VTbbQusWp_1xoHHj7L0Jc-smc&s=WBT-W8K2oCiDEud5rwj1xGVTxR0pcgYiQTt_xOJ_rl4&e= 
> 
> Files:
>  lib/Transforms/IPO/PassManagerBuilder.cpp
> 
> Index: lib/Transforms/IPO/PassManagerBuilder.cpp
> ===================================================================
> --- lib/Transforms/IPO/PassManagerBuilder.cpp
> +++ lib/Transforms/IPO/PassManagerBuilder.cpp
> @@ -244,7 +244,7 @@
>   // Start of function pass.
>   // Break up aggregate allocas, using SSAUpdater.
>   if (UseNewSROA)
> -    MPM.add(createSROAPass(/*RequiresDomTree*/ false));
> +    MPM.add(createSROAPass());
>   else
>     MPM.add(createScalarReplAggregatesPass(-1, false));
>   MPM.add(createEarlyCSEPass());              // Catch trivial redundancies
> 
> 
> <D12267.32915.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Dcommits&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=ygVmcuuQ1MUhRUoJm-IgPtgjmvM0byfjlHDg99vufEI&m=c4SYwnFJ6SDBptqMe1VTbbQusWp_1xoHHj7L0Jc-smc&s=GUrNm4tiqRjqQku2DQ2e1aEqZL7_j75PnDPgYwK6QOs&e=