This is an archive of the discontinued LLVM Phabricator instance.

[DomTree] Extend update API to allow a post CFG view.
ClosedPublic

Authored by asbirlea on Aug 6 2020, 1:30 PM.

Details

Summary

Extend the applyUpdates in DominatorTree to allow a post CFG view,
different from the current CFG.
This patch implements the functionality of updating an already up to
date DT, to the desired PostCFGView.
Combining a set of updates towards an up to date DT and a PostCFGView is
not yet supported.

Diff Detail

Event Timeline

asbirlea created this revision.Aug 6 2020, 1:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2020, 1:30 PM
asbirlea requested review of this revision.Aug 6 2020, 1:30 PM

I'm inclined to change the API to pass a second vector of updates instead of the GraphDiff directly (for the PostView), since during updates the GD gets modified and it makes sense to keep it as an internal component. Thoughts?

kuhar added a comment.Aug 11 2020, 9:24 AM

This makes sense to me. I think the only missing piece is some documentation which also explains how to interpret when PostView is missing (PostView == nullptr ===> Current CFG is the implicit PostView?)

llvm/include/llvm/Support/GenericDomTree.h
542–547

Can you update the comment?

553

nit: early return and not else?

asbirlea updated this revision to Diff 287107.Aug 21 2020, 2:25 PM
asbirlea marked 2 inline comments as done.

I made the update to keep the creation of the GD object internal to the DT. Since the GD is modified during updates, this makes the most sense to me.
I split the API to take only Updates or Updates+PostViewUpdates.

kuhar accepted this revision.Aug 21 2020, 2:29 PM
kuhar added inline comments.
llvm/include/llvm/Support/GenericDomTreeConstruction.h
567

nit: would it be possible to give it a more descriptive name? I have no idea what L stands for

This revision is now accepted and ready to land.Aug 21 2020, 2:29 PM
asbirlea added inline comments.Aug 21 2020, 2:55 PM
llvm/include/llvm/Support/GenericDomTreeConstruction.h
567

Good point, it was meant to be L for Local :-)

asbirlea updated this revision to Diff 287118.Aug 21 2020, 3:01 PM
asbirlea marked an inline comment as done.

Rename variable.

This revision was landed with ongoing or failed builds.Aug 21 2020, 5:24 PM
This revision was automatically updated to reflect the committed changes.