SESE Loops: transform loops to have single exit block in non-cloning case. #19385

bgogul · 2018-09-19T07:49:02Z

I could not break the PR further down. Hopefully the algorithm outline presented here helps.

Algorithm Outline

(Note that exiting blocks are inside the loop and exit blocks are outside the loop. Therefore, an exit edge is from an exiting block to an exit block.)

This PR takes care of transforming the loop such that the common post dominator of all exit blocks is the new exit block for the loop. This is one of the several transformations needed to eliminate undef from SESE loops. Consider the following snippet:

while(...) {
   if (...) {
     ... 
     break;
   }
}

Recall that the blocks within if do not belong to the while loop in the SIL IR. The transformation implemented in this PR has the effect of moving the blocks back into the loop.

Step 1. Transform Loop. (See ensureSingleExitBlock)

Let nearestCommonPD be the nearest common post-dominator block of all the exit blocks.
Move all the blocks reachable from exiting blocks to the nearestCommonPD inside the loop. (It is not always possible to move all reachable blocks without cloning some of the them. See notes below.)
Let exitBlocks be the set of exit blocks for the loop after the above transformation.

Step 2. Connect exiting blocks to a new latch and unify loop arguments. (See patchEdges)

(Only changes related to exit arguments are shown here.)

Let exitArgs be the union of the arguments of all exitBlocks.
for each exitArg in exitArgs, add an appropriate argument to the new header block. Let p0, p1, ..pn be the new arguments corresponding to exit arguments a0, a1, a2, ..., an.
For a exit edge br exit_i(x, y) -> exit_i(a2, a3), change the source to br new_latch(p0, p1, x, y, p4, ..., pn)

Notes.
If a block is not dominated by the header of the loop, we cannot move it into the loop in Step 1. For such cases, we will need to clone the block before moving it into the loop. I will send out a separate PR for that purpose.

Managing arguments

bgogul · 2018-09-19T07:49:21Z

@swift-ci please test tensorflow linux

bgogul · 2018-09-19T07:49:35Z

@swift-ci please test tensorflow macos

mhong · 2018-09-19T14:11:24Z

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

@@ -298,8 +302,9 @@ static SILValue createTFIntegerConst(GraphFunctionDeviceInfo &deviceInfo,
 class SingleExitLoopTransformer {


The PR descriptions gives a nice overview of how the code works. Would you like to also capture that as a comment block?

in the pr description, should br new_latch(p0, p1 be br new_latch(a0, a1?

No, it is br new_latch(p0, p1..). However there was a typo in the description. It should have been as follows: For an exit edge br exit_i(x, y) -> exit_i(a2, a3), change the source to br new_latch(p0, p1, x, y, p4, ..., pn). Essentially, we are passing x and y at the corresponding positions and use the state captured at the loop header for the other exit arguments. Intuitively, p_i captures the value for exit argument a_i at the new header.

(I also incorporated some of the text from the description into the comments.)

mhong · 2018-09-19T14:50:59Z

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

@@ -375,7 +389,7 @@ class SingleExitLoopTransformer {
  /// Equivalence classes induced by argument passing.
  llvm::EquivalenceClasses<SILValue> equivalentValues;
  /// exit blocks before the loop is transformed.


The PR description says:

Let exitBlocks be the set of exit blocks for the loop after the above transformation.

Is it before or after?

i believe it should before. (if it's "after", there would only be a single exit block.)

It is after the transformation in this PR. We can't have single exit block yet as we need to implement cloning.

mhong · 2018-09-19T14:58:24Z

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

@@ -323,6 +328,14 @@ class SingleExitLoopTransformer {
  llvm::DenseMap<SILValue, SILValue>
  getPreheaderSubstMap(const SmallPtrSetImpl<SILValue> &values) const;

+  /// Transform the loop by moving and cloning nodes (as needed) so
+  /// that the nearest common post dominator of the current exiting blocks


the current exiting blocks -> the current exit blocks?

mhong · 2018-09-19T14:58:42Z

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

@@ -323,6 +328,14 @@ class SingleExitLoopTransformer {
  llvm::DenseMap<SILValue, SILValue>
  getPreheaderSubstMap(const SmallPtrSetImpl<SILValue> &values) const;

+  /// Transform the loop by moving and cloning nodes (as needed) so
+  /// that the nearest common post dominator of the current exiting blocks
+  /// is a single exit block for the loop.


is -> becomes?

mhong · 2018-09-19T14:59:17Z

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

+      // top-level loop is already correct.
+    }
+    loop->addBasicBlockToLoop(outsideBlock, LI->getBase());
+  }


should we do a sanity check that loop->getExitBlocks() should return a single block here?

Not yet, as we will need to implement cloning of blocks as mentioned in the PR description.

mhong · 2018-09-19T15:03:53Z

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

-      canonicalizeAllLoops(&DI, &LI);
+      if (canonicalizeAllLoops(&DI, &LI)) {
+        // Recalculate PDI if canonicalization made any changes.
+        PDI.recalculate(*F);


we already have PDI.recalculate(*F); when if (loopChanged). why do we need this additional call?

This is to take care of changes introduced by canonicalizeAllLoops, which happens before our transformations. There is no support to incrementall update PDI unlike DI and LI.

It seems we are not using PDI before this call. should we remove PDI(F) from the init list in c'tor, so that the call here becomes its initialization? That would make the code more readable for me, but you can decide.

also consider moving the canonicalizeAllLoops() call to the c'tor (so that PDI initialization code is done as part of initialization).

i also noticed processAcyclicRegionExcludingEnd() may not need to be public.

I will leave the PDI as is for now. I plan to move the SingleExitTransformer class to a separate file as it is getting larger. I will do some of the refactoring then.

I made the non-interface methods private.

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

lattner · 2018-09-19T16:08:26Z

From a quick read of the code (not a deep algorithmic understanding) the patch LGTM!

bgogul

Thanks for the review.

bgogul · 2018-09-19T21:05:59Z

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

-      canonicalizeAllLoops(&DI, &LI);
+      if (canonicalizeAllLoops(&DI, &LI)) {
+        // Recalculate PDI if canonicalization made any changes.
+        PDI.recalculate(*F);


This is to take care of changes introduced by canonicalizeAllLoops, which happens before our transformations. There is no support to incrementall update PDI unlike DI and LI.

bgogul · 2018-09-19T21:08:43Z

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

@@ -298,8 +302,9 @@ static SILValue createTFIntegerConst(GraphFunctionDeviceInfo &deviceInfo,
 class SingleExitLoopTransformer {


No, it is br new_latch(p0, p1..). However there was a typo in the description. It should have been as follows: For an exit edge br exit_i(x, y) -> exit_i(a2, a3), change the source to br new_latch(p0, p1, x, y, p4, ..., pn). Essentially, we are passing x and y at the corresponding positions and use the state captured at the loop header for the other exit arguments. Intuitively, p_i captures the value for exit argument a_i at the new header.

(I also incorporated some of the text from the description into the comments.)

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

bgogul · 2018-09-19T21:09:30Z

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

+      // top-level loop is already correct.
+    }
+    loop->addBasicBlockToLoop(outsideBlock, LI->getBase());
+  }


Not yet, as we will need to implement cloning of blocks as mentioned in the PR description.

bgogul · 2018-09-19T21:10:04Z

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

@@ -323,6 +328,14 @@ class SingleExitLoopTransformer {
  llvm::DenseMap<SILValue, SILValue>
  getPreheaderSubstMap(const SmallPtrSetImpl<SILValue> &values) const;

+  /// Transform the loop by moving and cloning nodes (as needed) so
+  /// that the nearest common post dominator of the current exiting blocks


bgogul · 2018-09-19T21:10:09Z

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

@@ -323,6 +328,14 @@ class SingleExitLoopTransformer {
  llvm::DenseMap<SILValue, SILValue>
  getPreheaderSubstMap(const SmallPtrSetImpl<SILValue> &values) const;

+  /// Transform the loop by moving and cloning nodes (as needed) so
+  /// that the nearest common post dominator of the current exiting blocks
+  /// is a single exit block for the loop.


bgogul · 2018-09-19T21:11:22Z

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

@@ -375,7 +389,7 @@ class SingleExitLoopTransformer {
  /// Equivalence classes induced by argument passing.
  llvm::EquivalenceClasses<SILValue> equivalentValues;
  /// exit blocks before the loop is transformed.


It is after the transformation in this PR. We can't have single exit block yet as we need to implement cloning.

bgogul · 2018-09-20T00:16:38Z

I will go ahead and merge the PR.

bgogul added 4 commits September 17, 2018 18:18

Move blocks into the loop as much as possible in common cases.

66593f6

Managing arguments

Split edges into exit blocks that do not dominate the header of loop.

c8b1c50

Updated the tests

c23a7b0

Guarded new code with a flag.

98e7a06

bgogul requested review from rxwei, mhong and lattner September 19, 2018 07:51

mhong approved these changes Sep 19, 2018

View reviewed changes

Added documentation from the PR description.

60493b1

bgogul commented Sep 19, 2018

View reviewed changes

Make some methods private

d587daa

bgogul merged commit 8c300b2 into apple:tensorflow Sep 20, 2018

bgogul deleted the reduce_exit_blocks branch September 20, 2018 00:19

swift-ci mentioned this pull request Sep 24, 2018

[SR-7765] Lack of support for imperfect loop exits blocking simpleCounterLoop test #50304

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SESE Loops: transform loops to have single exit block in non-cloning case. #19385

SESE Loops: transform loops to have single exit block in non-cloning case. #19385

bgogul commented Sep 19, 2018 •

edited

bgogul commented Sep 19, 2018

bgogul commented Sep 19, 2018

mhong Sep 19, 2018

mhong Sep 19, 2018

bgogul Sep 19, 2018 •

edited

mhong Sep 19, 2018

bgogul Sep 19, 2018

mhong Sep 19, 2018

bgogul Sep 19, 2018

mhong Sep 19, 2018

bgogul Sep 19, 2018

mhong Sep 19, 2018

bgogul Sep 19, 2018

mhong Sep 19, 2018

bgogul Sep 19, 2018

mhong Sep 19, 2018

mhong Sep 19, 2018

bgogul Sep 20, 2018

lattner commented Sep 19, 2018

bgogul left a comment

bgogul Sep 19, 2018

bgogul Sep 19, 2018 •

edited

bgogul Sep 19, 2018

bgogul Sep 19, 2018

bgogul Sep 19, 2018

bgogul Sep 19, 2018

bgogul commented Sep 20, 2018

		@@ -298,8 +302,9 @@ static SILValue createTFIntegerConst(GraphFunctionDeviceInfo &deviceInfo,
		class SingleExitLoopTransformer {

SESE Loops: transform loops to have single exit block in non-cloning case. #19385

SESE Loops: transform loops to have single exit block in non-cloning case. #19385

Conversation

bgogul commented Sep 19, 2018 • edited

Algorithm Outline

bgogul commented Sep 19, 2018

bgogul commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgogul Sep 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lattner commented Sep 19, 2018

bgogul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgogul Sep 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgogul commented Sep 20, 2018

bgogul commented Sep 19, 2018 •

edited

bgogul Sep 19, 2018 •

edited

bgogul Sep 19, 2018 •

edited