Page MenuHomePhabricator

Separate ExecutionDepsFix into 4 parts - enable breaking false dependencies for all reg classes.
ClosedPublic

Authored by myatsina on Nov 21 2017, 10:21 PM.

Details

Summary

Separate ExecutionDepsFix into 4 parts:

  1. ReachingDefsAnalysis - Allows to identify for each instruction what is the “closest” reaching def of a certain register. Used by BreakFalseDeps (for clearance calculation) and ExecutionDomainFix (for arbitrating conflicting domains).
  2. ExecutionDomainFix - Changes the variant of the instructions in order to minimize domain crossings.
  3. BreakFalseDeps - Breaks false dependencies.
  4. LoopTraversal - Creatws a traversal order of the basic blocks that is optimal for loops (introduced in revision L293571). Both ExecutionDomainFix and ReachingDefsAnalysis use this to determine the order they will traverse the basic blocks.

This also included the following changes to ExcecutionDepsFix original logic:

  1. BreakFalseDeps and ReachingDefsAnalysis logic no longer restricted by a register class.
  2. ReachingDefsAnalysis tracks liveness of reg units instead of reg indices into a given reg class.

Additional changes in affected files:

  1. X86 and ARM targets now inherit from ExecutionDomainFix instead of ExecutionDepsFix. BreakFalseDeps also was added to the passes they activate.
  2. Comments and references to ExecutionDepsFix replaced with ExecutionDomainFix and BreakFalseDeps, as appropriate.

Additional refactoring changes will follow.

This commit is (almost) NFC.
The only functional change is that now BreakFalseDeps will break dependency for all register classes.
Since no additional instructions were added to the list of instructions that have false dependencies, there is no actual change yet.
In a future commit several instructions (and tests) will be added.

This is the first of 5 patches that fix bugzilla https://bugs.llvm.org/show_bug.cgi?id=33869

Next patches:
https://reviews.llvm.org/D40331
https://reviews.llvm.org/D40332
https://reviews.llvm.org/D40333
https://reviews.llvm.org/D40334

Diff Detail

Repository
rL LLVM

Event Timeline

myatsina created this revision.Nov 21 2017, 10:21 PM
myatsina set the repository for this revision to rL LLVM.
myatsina added a subscriber: llvm-commits.
myatsina edited the summary of this revision. (Show Details)Nov 22 2017, 2:29 AM
myatsina added a reviewer: zvi.
zvi edited edge metadata.Nov 22 2017, 9:26 AM

LGTM with a minor comment. Thanks!

lib/CodeGen/ExecutionDepsFix.cpp
183 ↗(On Diff #123863)

This is more compact and safe:

Defs.assign(numRegUnits, DefaultDefVal);
myatsina updated this revision to Diff 124091.Nov 23 2017, 8:09 AM
myatsina marked an inline comment as done.

Fixed vector intialization.
Also incorporated comments from https://reviews.llvm.org/D40333 in this patch, which seems to be more appropriate than the later one.

myatsina updated this revision to Diff 126844.Dec 13 2017, 2:35 PM
myatsina retitled this revision from Separate ExecutionDepsFix into 3 parts - enable breaking false dependencies for all reg classes. to Separate ExecutionDepsFix into 4 parts - enable breaking false dependencies for all reg classes..
myatsina edited the summary of this revision. (Show Details)

Changes based on comments in https://reviews.llvm.org/D40333

Changed LoopTraversal to not be a pass, but rather a class which returns the BB order and other passes can then traverse and process the BBs.
Separated some logic to a ReachingDefAnalysis pass which is used by both BreakFalsDeps and ExecutionDomainFix.

atdt added a subscriber: atdt.Dec 15 2017, 1:08 PM
craig.topper added inline comments.Jan 10 2018, 11:54 AM
include/llvm/CodeGen/ExecutionDepsFix.h
136 ↗(On Diff #126844)

twise->twice

170 ↗(On Diff #126844)

Remove the const. It doesn't mean anything if you're returning a whole object.

lib/CodeGen/ExecutionDepsFix.cpp
878 ↗(On Diff #126844)

traverse is returning an object not a reference so we shouldn't be capturing by reference.

You may want to consider having the caller pass a SmallVector to traverse. That way traverse doesn't have to hardcode an explicit size for the SmallVector.

897 ↗(On Diff #126844)

Add blank line before this function

This revision was not accepted when it landed; it landed in state Needs Review.Jan 22 2018, 2:07 AM
This revision was automatically updated to reflect the committed changes.
myatsina marked 4 inline comments as done.

I've addressed the comments in this review and done the commit based on the LGTM in https://reviews.llvm.org/D40333, which is the "end version" of all these refactoring changes after more refactoring and clang-format.

lib/CodeGen/ExecutionDepsFix.cpp
878 ↗(On Diff #126844)

I've removed in the commit the redundant consts and references as you suggested.
In a later commit I define a dedicated type that obscures the SmallVector size (you can see it in the https://reviews.llvm.org/D40333 review).