Page MenuHomePhabricator

BranchFolding: Use LivePhysReg to update live in lists.

Authored by MatzeB on Jul 5 2016, 6:24 PM.



Use LivePhysRegs with a backwards walking algorithm to update live in
lists, this way the results do not depend on the presence of kill flags

This patch also reduces the number of registers added as live-in. Previously all pristine registers as well as all sub registers of a super register were added resulting in unnecessarily large live in lists (the block noticed in the discussion of may have been caused by this).

Diff Detail


Event Timeline

MatzeB updated this revision to Diff 62809.Jul 5 2016, 6:24 PM
MatzeB retitled this revision from to BranchFolding: Use LivePhysReg to update live in lists..
MatzeB updated this object.
MatzeB added reviewers: qcolombet, ab.
MatzeB set the repository for this revision to rL LLVM.
MatzeB added a subscriber: llvm-commits.
mcrosier added a comment.EditedJul 6 2016, 8:04 AM

Minor nit.

416 ↗(On Diff #62809)

Pre-increment of I is preferred.

MatzeB updated this revision to Diff 63000.Jul 6 2016, 5:13 PM
MatzeB updated this object.

Address nitpicks, add a comment to explain the purpose of the loop over the super registers.

ab added inline comments.Jul 11 2016, 9:23 AM
412 ↗(On Diff #63000)

Unnecessary braces?

415–428 ↗(On Diff #63000)

Would it make sense to iterate on the LivePhysRegs set instead?

106 ↗(On Diff #63000)

Maybe remove LiveRegs from here, and only create it in computeLiveIns? It's cleared anyway, and looks cheap to construct.

MatzeB marked 3 inline comments as done.Jul 11 2016, 11:48 AM

Thanks for the review!

415–428 ↗(On Diff #63000)

good idea.

106 ↗(On Diff #63000)

It is still a bigger heap allocation for the Sparse memory of the SparseSet. Heap allocations are not completely free. We probably won't notice the difference in this case, but keeping a member in the enclosing pass isn't a big thing either.

I did however move the initialization into the computeLiveIn() function as I realized that there is a good chance that for simpler function we will never end up calling it.

MatzeB updated this revision to Diff 63545.Jul 11 2016, 11:48 AM
MatzeB marked an inline comment as done.

Address review comments.

ab accepted this revision.Jul 12 2016, 6:56 AM
ab edited edge metadata.


25 ↗(On Diff #63545)

Since it's in the .h already, remove it from here?

415 ↗(On Diff #63545)

false -> /*IncludeSelf=*/ false, or even remove it?

433 ↗(On Diff #63545)

stale comment (here and later)

106 ↗(On Diff #63545)

Sounds good; I thought init() re-did the allocation, but that's not true.

This revision is now accepted and ready to land.Jul 12 2016, 6:56 AM
This revision was automatically updated to reflect the committed changes.