This is an archive of the discontinued LLVM Phabricator instance.

CodeGen: Refactor renameDisconnectedComponents() as a pass
ClosedPublic

Authored by MatzeB on May 20 2016, 8:08 PM.

Details

Summary

Refactor LiveIntervals::renameDisconnectedComponents() to be a pass.
Also change the name to "RenameIndependentSubregs":

  • renameDisconnectedComponents() worked on a MachineFunction at a time so it is a natural candidate for an machine function pass.
  • The algorithm is testable with a .mir test now.
  • This also fixes a problem where the lazy renaming as part of the MachineScheduler introduced IMPLICIT_DEF instructions after the number of a nodes in a region were counted leading to a mismatch.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 58029.May 20 2016, 8:08 PM
MatzeB retitled this revision from to CodeGen: Refactor renameDisconnectedComponents() as a pass.
MatzeB updated this object.
MatzeB added a reviewer: qcolombet.
MatzeB set the repository for this revision to rL LLVM.
MatzeB added a subscriber: llvm-commits.
MatzeB updated this revision to Diff 58030.May 20 2016, 8:09 PM

Add a .mir test for the RenameIndependentSubregs pass.

MatzeB updated this revision to Diff 58321.May 24 2016, 2:41 PM
MatzeB edited edge metadata.

Actually include newly created RenameIndependentSubregs.cpp file.

qcolombet edited edge metadata.May 26 2016, 5:46 PM

Hi Matthias,

I’ll try to look at this next week.

Thanks for your patience!

Cheers,
-Quentin

qcolombet accepted this revision.May 31 2016, 2:15 PM
qcolombet edited edge metadata.

Hi Matthias,

LGTM.

Nitpicks inlined.

Cheers,
-Quentin

lib/CodeGen/LiveRangeUtils.h
10 ↗(On Diff #58321)

typo: functions

22 ↗(On Diff #58321)

typo: to to

lib/CodeGen/RenameIndependentSubregs.cpp
1 ↗(On Diff #58321)

Copy/paste :)

40 ↗(On Diff #58321)

Could you add a DEBUG_TYPE and a few DEBUG statements in the pass to follow what is going on?

107 ↗(On Diff #58321)

Period

122 ↗(On Diff #58321)

DEBUG comment.
Like we could print the register here.

127 ↗(On Diff #58321)

DEBUG comment.
And add a comment here to say we are going to check if that live-range has several component, etc.

137 ↗(On Diff #58321)

Although the current implementation is correct, I was wondering what are your thoughts regarding relaxing the constraints on the register class and constraining them back when we rewrite the operands?
I actually do not expect that we would get much more freedom for regalloc afterwards, but who knows.

Note: the current implementation is the right thing to do, I was thinking in terms of follow-up patch.

201 ↗(On Diff #58321)

DEBUG comment.
Print that number.

test/CodeGen/AMDGPU/rename-independent-subregs.mir
6 ↗(On Diff #58321)

Could you add a comment explaining what configuration does this test check?
Basically, I would expecting something stating that this test checks that we correctly split %0 as it has three disjoint sets.

This revision is now accepted and ready to land.May 31 2016, 2:15 PM
MatzeB marked 10 inline comments as done.May 31 2016, 3:38 PM
MatzeB added inline comments.
lib/CodeGen/RenameIndependentSubregs.cpp
40 ↗(On Diff #58321)

I added some DEBUG printing that tells you how many equivalence classes were found and into which newly created vregs they were distributed. I opted to not print anything for vregs where no independent classes could be found to keep the debug output small.

107 ↗(On Diff #58321)

I kept this as it is the officially recommended way in the llvm coding convention and what is done in most parts of llvm. I know it contradicts that other rule in the coding convention that states that you should write sentences ending in periods in your comments...

137 ↗(On Diff #58321)

Yes this indeed something we could do in a follow-up commit, I added a TODO comment.
And for the record: We have the same issue in the LiveIntervals::splitSeparateComponents() code.

This revision was automatically updated to reflect the committed changes.