This is an archive of the discontinued LLVM Phabricator instance.

[IPSCCP,PM] Preserve DT in the new pass manager.
ClosedPublic

Authored by fhahn on May 23 2018, 7:25 AM.

Details

Summary

After D45330, Dominators are required for IPSCCP and can be preserved.

This patch preserves DominatorTreeAnalysis in the new pass manager. AFAIK the legacy pass manager cannot preserve function analysis required by a module analysis.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.May 23 2018, 7:25 AM
fhahn updated this revision to Diff 173164.Nov 8 2018, 7:11 AM
fhahn retitled this revision from [IPSCCP] Preserve DT in the new pass manager. to [IPSCCP,PM] Preserve DT in the new pass manager..
fhahn edited the summary of this revision. (Show Details)

Rebased after DTUpdater changes landed.

This patch also restructures the way analyses are passed between the solver and IPO/SCCP.cpp, to make it slightly simpler.

fhahn added a reviewer: kuhar.Nov 8 2018, 7:12 AM
kuhar added inline comments.
lib/Transforms/Scalar/SCCP.cpp
2002 ↗(On Diff #173164)

Does it make sense to have a SmallVector of such size here? I can see that this code wasn't introduce in this patch, but wanted to note it anyway.

2052 ↗(On Diff #173164)

Why is DTU passed to this function in a few of lines above but not here?

fhahn updated this revision to Diff 173173.Nov 8 2018, 8:52 AM

Fixed test case (new pm needs -passes='ipsccp,function(verify<domtree>) to verify the DTs afterwards), also pass DTU to other changeToUnreachable and ConstantFoldTerminator calls.

fhahn marked an inline comment as done.Nov 8 2018, 8:53 AM
fhahn added inline comments.
lib/Transforms/Scalar/SCCP.cpp
2002 ↗(On Diff #173164)

I am not sure, but I think that would be something to look at independently of this patch.

kuhar accepted this revision.Nov 8 2018, 10:04 AM

Looks good to me now, thanks for the fixes! You may want for somebody else to take a look at it, though.

Have you looked into preserving PDT if available? It should be easy with DTU, but I don't know if it makes sense to preserve it in this place of the pipeline.

This revision is now accepted and ready to land.Nov 8 2018, 10:04 AM
NutshellySima added inline comments.Nov 8 2018, 10:23 AM
lib/Transforms/IPO/SCCP.cpp
24 ↗(On Diff #173173)

It is also possible to preserve PostDominatorTree here trivially using DTU(DT, PDT, ...) and auto* PDT = FAM.getCachedResult.

lib/Transforms/Scalar/SCCP.cpp
2080 ↗(On Diff #173173)

I would suggest using DTU.deleteBB(DeadBB) here which delays BB removal until flush() and removing

for (BasicBlock *DeadBB : BlocksToErase)
      F.getBasicBlockList().erase(DeadBB);

below.
I believe it would make the code more readable.

LGTM and I'm glad to see other passes working to preserve DTU.

lib/Transforms/Scalar/SCCP.cpp
2080 ↗(On Diff #173173)

+1 and it also defers a potentially expensive call to flush().

chandlerc accepted this revision.Nov 8 2018, 11:02 AM

LGTM, nit below.

include/llvm/Transforms/Scalar/SCCP.h
42–43 ↗(On Diff #173173)

s/Analysis/AnalysisResults/ ?

fhahn updated this revision to Diff 173205.Nov 8 2018, 11:47 AM
fhahn marked 3 inline comments as done.

Thank you very much for having a look! I updated the patch to use deleteBB, added a comment in lib/Transform/IPO/SCCP.cpp on why nullptr is passed in as DT for the legacy pass manager and renamed AnalysisForFn to AnalysisResultsForFn.

Unless there are any additional comments, I plan to commit this tomorrow.

include/llvm/Transforms/Scalar/SCCP.h
42–43 ↗(On Diff #173173)

Thanks, that makes more sense. I also renamed AnalysisInfo to AnalysisResults in SCCP.cpp

lib/Transforms/Scalar/SCCP.cpp
2080 ↗(On Diff #173173)

Neat, thank you very much!

fhahn added inline comments.Nov 8 2018, 11:50 AM
lib/Transforms/IPO/SCCP.cpp
24 ↗(On Diff #173173)

IPSCCP currently runs relatively early, but if beneficial it can be added in a follow up patch quite easily by extending the AnalysisResultsForFn struct.

chandlerc accepted this revision.Nov 8 2018, 11:53 AM

FWIW, still LGTM and I think you can go ahead and submit.

I would like to see PDT preserved as well (as suggested in review) and maybe other improvements, but all of those seem totally fine for follow-up patches.

fhahn added a comment.Nov 8 2018, 1:56 PM

FWIW, still LGTM and I think you can go ahead and submit.

Great, thank you very much for the prompt review!

I would like to see PDT preserved as well (as suggested in review) and maybe other improvements, but all of those seem totally fine for follow-up patches.

I will prepare a patch for preserving PDT tomorrow. I am more than happy to take a look at any follow up improvements. Do you have anything particular in mind, besides preserving PDT?

This revision was automatically updated to reflect the committed changes.