Page MenuHomePhabricator

Rename determineInsertionPoints to determinePHINodes, expose it so it can be reused
ClosedPublic

Authored by dberlin on Apr 20 2015, 11:49 AM.

Details

Summary

MemorySSA uses this algorithm. This moves the code around and changes the arguments so it can be reused in both places.
We don't have an SSAUtils.h or anything, but happy to put this wherever.

There are no actual algorithm or datastructure changes in here, just code movement.

Diff Detail

Repository
rL LLVM

Event Timeline

dberlin retitled this revision from to Rename determineInsertionPoints to determinePHINodes, expose it so it can be reused.
dberlin updated this object.
dberlin edited the test plan for this revision. (Show Details)
dberlin added reviewers: qcolombet, chandlerc.
dberlin added a subscriber: Unknown Object (MLST).
dberlin added inline comments.Apr 20 2015, 12:04 PM
lib/Transforms/Utils/PromoteMemoryToRegister.cpp
1008 ↗(On Diff #24039)

Sorry, accidentally carried this comment from the invocation, being rewritten now. :)

dberlin added inline comments.Apr 20 2015, 12:06 PM
lib/Transforms/Utils/PromoteMemoryToRegister.cpp
1014 ↗(On Diff #24039)

Note that i have deliberately not changed the algorithm handling numbering here. Post-this-patch, i will move the block number lookups and sorting into the caller, and make this a simple SmallVector<BasicBlock *>, and remove the BBNumbers argument)

  • Update comments
qcolombet edited edge metadata.Apr 20 2015, 2:28 PM

Hi Daniel

include/llvm/Transforms/Utils/PromoteMemToReg.h
70 ↗(On Diff #24046)

From the user stand point I think we should not have to provide the DomLevels and the LiveInBlocks.

The BBNumbers are already on your “to-remove” list, so it is fine for now.

dberlin edited edge metadata.
  • Update to move IDF calculation
qcolombet accepted this revision.Apr 21 2015, 10:07 AM
qcolombet edited edge metadata.

Hi Daniel,

Thanks for the refactoring!

LGTM with a couple of nitpicks.

Cheers,
-Quentin

include/llvm/Analysis/IteratedDominanceFrontier.h
45 ↗(On Diff #24136)

Add a comment that says that Blocks must be valid for the whole lifetime of the IDFCalculator (or the next call to setDefiningBlocks).

49 ↗(On Diff #24136)

Ditto.

58 ↗(On Diff #24136)

Add a comment on what will IDFBlocks contain.

Edit: I see that the comment is in the cpp file. I believe it would be more convenient to have it here. Also, does doxygen pick up the comments from the .cpp?

This revision is now accepted and ready to land.Apr 21 2015, 10:07 AM
This revision was automatically updated to reflect the committed changes.