This is an archive of the discontinued LLVM Phabricator instance.

[CallSiteSplitting] Preserve DominatorTreeAnalysis.
ClosedPublic

Authored by fhahn on Feb 12 2018, 1:27 AM.

Details

Summary

The dominator tree analysis can be preserved easily.
Some other kinds of analysis can probably be preserved
too.

Diff Detail

Event Timeline

fhahn created this revision.Feb 12 2018, 1:27 AM
junbuml added inline comments.Feb 12 2018, 8:59 AM
lib/Transforms/Scalar/CallSiteSplitting.cpp
410

Considering that this pass doesn't use DT, do we need to get DT to preserve DT ?

fhahn added inline comments.Feb 12 2018, 9:00 AM
lib/Transforms/Scalar/CallSiteSplitting.cpp
410

We need the current dominator tree, so we can update it. Not sure if there is a better way to get it though.

junbuml added inline comments.Feb 12 2018, 9:15 AM
lib/Transforms/Scalar/CallSiteSplitting.cpp
410

I just wanted to ask if constructing DT in advance is just okay because this pass doesn't even use DT. Isn't it okay to construct DT when DT is really required in a later pass?

fhahn added inline comments.Feb 12 2018, 9:18 AM
lib/Transforms/Scalar/CallSiteSplitting.cpp
410

I'll check. AFAIK CallSiteSplitting is run after a bunch of passes that require the DT to be constructed already and by saying we do not preserve the DT we force it to be re-computed.

brzycki added inline comments.
lib/Transforms/Scalar/CallSiteSplitting.cpp
410

Hi @fhahn , you should just need one additional line in your patch to inform the pass manager to properly preserve DT.

INITIALIZE_PASS_DEPENDENCY(DominatorTreeWrapperPass)

You can use -debug-pass=Executions with opt during testing to verify the order of creation/deletion of the Dominator Tree pass. Here's an example snippet:

[2018-02-12 11:37:22.659888285] 0x1fb5240     Executing Pass 'Dominator Tree Construction' on Function 'test1'...
[2018-02-12 11:37:22.659992339] 0x1fb5240     Executing Pass 'Basic Alias Analysis (stateless AA impl)' on Function 'test1'...
[2018-02-12 11:37:22.660156849] 0x1fb5240     Executing Pass 'Function Alias Analysis Results' on Function 'test1'...
[2018-02-12 11:37:22.660254827] 0x1fb5240     Executing Pass 'Lazy Value Information Analysis' on Function 'test1'...
[2018-02-12 11:37:22.660302165] 0x1fb5240     Executing Pass 'Jump Threading' on Function 'test1'...
[2018-02-12 11:37:22.661679568] 0x1fb5240     Made Modification 'Jump Threading' on Function 'test1'...
[2018-02-12 11:37:22.661739504] 0x1fb5240      Freeing Pass 'Basic Alias Analysis (stateless AA impl)' on Function 'test1'...
[2018-02-12 11:37:22.661767275] 0x1fb5240      Freeing Pass 'Function Alias Analysis Results' on Function 'test1'...
[2018-02-12 11:37:22.661795430] 0x1fb5240      Freeing Pass 'Lazy Value Information Analysis' on Function 'test1'...
[2018-02-12 11:37:22.661840127] 0x1fb5240      Freeing Pass 'Jump Threading' on Function 'test1'...
[2018-02-12 11:37:22.661870858] 0x1fb5240      Freeing Pass 'Dominator Tree Construction' on Function 'test1'...
[2018-02-12 11:37:22.661904074] 0x1fb5240     Executing Pass 'Module Verifier' on Function 'test1'...

Please use getAnalysisIfAvailable for the old pass manager
Please use getCachedResult for the new pass manager.

That is the correct way to "get an analysis if it is sitting around so we can update it".

The way you've done it now will actually calculate DT if it's not calculated already.

fhahn updated this revision to Diff 133897.Feb 12 2018, 10:28 AM

Thanks @dberlin .

updated to use use getAnalysisIfAvailable for the old pass manager and getCachedResult for the new pass manager.

fhahn updated this revision to Diff 133900.Feb 12 2018, 10:50 AM

Mark DominatorTreeWrapperPass as preserved with old pass manager.

So this looks right to me.
I'm a little worried about how we are having to modify tons and tons of functions to push these through.
When we get to postdominator updates, we'll have to do the same thing.

At some point, if what we are trying to do is preserve analysis, i wonder if we shouldn't be passing analysismanager's around or something.

dberlin accepted this revision.Feb 13 2018, 10:22 AM
This revision is now accepted and ready to land.Feb 13 2018, 10:22 AM

An example of that is getBestSimplifyQuery, which takes various analysis managers, etc, and extracts the analysis it cares about them.
If we did similar, we could simplify what gets passed through and easily add updating of a new thing.

fhahn added a comment.Feb 14 2018, 2:37 PM

Thanks for the pointers. Do you think it would make sense to add something generic which takes various analysis managers, etc, and extracts the analysis on demand, for other passes to use too?

Personally, i totally think this is worthwhile and worth spending some time on to prevent the slow proliferation of 10+ argument functions taking analysis results. Because that's the path we are heading down, and that's definitely hard to refactor, change, etc
But i'm also not the person who would do the work (though happy to review).

If you want to look into it, bring it up on the mailing list and see what folks think.

fhahn updated this revision to Diff 138935.Mar 19 2018, 8:22 AM

Rebased.

fhahn updated this revision to Diff 139433.Mar 22 2018, 6:25 AM

rebased again

This revision was automatically updated to reflect the committed changes.