Page MenuHomePhabricator

[X86] Refactor DomainReassignment pass to make the Closure class not stores references to the main data structures of the pass itself
ClosedPublic

Authored by craig.topper on Dec 16 2017, 7:26 PM.

Details

Summary

Multiple Closure objects can be created and stored for a single function. It's not a good idea to devote so many fields of it to storing pointers and references to global data structures of the pass. The closure class should only store the things needed to represent the closure itself.

This pass refactors many of the methods of Closure to belong to the pass object and to pass around a reference to the current Closure. The Closure class gains a few simple methods to add instructions and edges, and to return iterators to edges and instructions.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Dec 16 2017, 7:26 PM
guyblank added inline comments.Dec 17 2017, 8:56 AM
lib/Target/X86/X86DomainReassignment.cpp
311 ↗(On Diff #127265)

i don't see this in top of trunk, is this the right diff?

craig.topper added inline comments.Dec 17 2017, 10:04 AM
lib/Target/X86/X86DomainReassignment.cpp
311 ↗(On Diff #127265)

Should have been committed in r320940.

guyblank added inline comments.Dec 18 2017, 12:13 AM
lib/Target/X86/X86DomainReassignment.cpp
311 ↗(On Diff #127265)

yes, sorry.

317 ↗(On Diff #127265)

IMO the domain should remain part of the closure itself. as opposed to the other removed fields, it really is a property of the closure.

328 ↗(On Diff #127265)

typo
close sure -> closure

338 ↗(On Diff #127265)

the function can be const

396 ↗(On Diff #127265)

Reassign -> reassign

craig.topper added inline comments.Dec 18 2017, 10:58 AM
lib/Target/X86/X86DomainReassignment.cpp
317 ↗(On Diff #127265)

I removed it because it was only used during buildClosure and wasn't needed after that. So I just made it a local variable in buildClosure and passed it by reference to visitRegister.

Address review comments except for moving the Domain into the Closure since its so short lived.

I also fixed a few additional places that should have been const.

guyblank accepted this revision.Dec 18 2017, 10:56 PM

LGTM, thanks.

This revision is now accepted and ready to land.Dec 18 2017, 10:56 PM
This revision was automatically updated to reflect the committed changes.