SESE: deal with the case where outer loops are moved into the current loop. #20222

bgogul · 2018-11-01T19:02:37Z

Consider the following example:

while ... {
  if ... {
     for(...) {...} // This should be nested into the while loop.
     break;
  }
}

In the SIL CFG, the statement in the if block including the for loop will not be part of the outer while loop. One of the steps during SESE canonicalization is to transform the CFG so that the statements in the if block become part of the while loop. This PR takes care of this case by doing the following:

Canonicalizing the for loop first.
Nesting the canonicalized for loop into the while loop and updating LoopInfo as needed.

This also takes care of the case with loops analogous to the situation handled in #19570 (See the example added to sese_loop_canonicalization.sil in this PR.)

This is the final PR for https://bugs.swift.org/browse/SR-7765 (modulo unknown bugs).

bgogul · 2018-11-01T19:10:51Z

@swift-ci please test tensorflow

mhong

Congrats on completing the feature and bug fix work!

(There's still some clean-up work remaining, but pls feel free to resolve the bug first if you prefer.)

mhong · 2018-11-01T20:21:07Z

test/TensorFlow/sese_loop_canonicalization.swift

+  let maxCount: Int32 = 100
+  while i <= maxCount {
+    sum += i
+    if (i == breakIndex) {


remove (). same below

mhong · 2018-11-01T20:22:16Z

test/TensorFlow/sese_loop_canonicalization.swift

+
+// CHECK-LABEL: --- XLA CFG Loops Before Canonicalize: {{.*}}testLoopMovementFromOutside{{.*}}
+// CHECK: Loop at depth 1 containing: {{.*}}
+// CHECK: Loop at depth 1 containing: {{.*}}


add a comment that this is the inner while loop above, but it's considered unnested from the outer while from loop info perspective.

mhong · 2018-11-01T20:23:58Z

test/TensorFlow/sese_loop_canonicalization.sil

@@ -361,3 +361,88 @@ bb8:
 bb9(%18 : $Builtin.Int32):
  return %18 : $Builtin.Int32
 }
+
+// The following example is similar to loopThatRequiresNodeCloning, where the


given the swift level test in the next file below, what is the value of having this SIL level test?

I feel it could take non-trivial work to read and maintain such tests.

This is representing a case where we need to move an outside loop into the currently processed loop. However, the header of the outside loop is not dominated by the currently processed loop. In this case, we will have to clone the loop and make sure the LoopInfo is updated.

We want a very specific CFG to test this scenario. See the ASCII graph in the test file for the CFG. With a swift file, the generated CFG can change depending upon the version of the compiler. (Also, the compiler did generate a similar CFG for an example.)

mhong · 2018-11-01T20:25:28Z

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

-    assert(latch && "Canonicalization should have given us one latch block");
-    initialize();
+  public:
+    static bool doIt(GraphFunctionDeviceInfo *deviceInfo, SILLoopInfo *LI,


document the return value

mhong · 2018-11-01T20:38:13Z

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

+  //   }
+  SmallPtrSet<SILBasicBlock *, 32> unrelatedPreheaders;
+  for (auto *otherLoop : *LI) {
+    // If loop and otherLoop are not nested into either, remember the preheader.


consider moving this comment right above line 745, to describe the semantics of unrelatedPreheaders.

Here the comment feels more "operational", and a more "declarative" comment on the semantics / spec of unrelatedPreheaders could be more useful.

mhong · 2018-11-01T20:48:15Z

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

+        // the entire loop now so that we can also incrementally update LoopInfo.
+        if (succBlockLoop) {
+          SILLoop *clonedLoop =
+              cloner.cloneLoop(LI, succBlockLoop, succBlockLoop->getHeader());


based on earlier discussion, should this API be renamed to sth like cloneOrUnroll?

Done and updated the comment as well.

mhong · 2018-11-01T21:00:08Z

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

-      outsideBlockLoop->removeBlockFromLoop(outsideBlock);
-      LI->changeLoopFor(outsideBlock, nullptr);
+      if (outsideBlockLoop->contains(loop)) {
+        // If our `loop` is nested within `outsideBlockLoop`.  Move the node


is there test coverage on both this if branch, and the else below?

Yes, we have coverage for both. The if branch was handled even before this PR (and is covered by the example with labeled breaks).

The else branch is the new behavior and the tests in this PR provide coverage.

bgogul · 2018-11-02T22:47:41Z

@swift-ci please test tensorflow

bgogul · 2018-11-02T23:42:03Z

Thanks for the review, Ming. Merging it now.

bgogul changed the title ~~[WIP] SESE: deal with the case where outer loops are moved into the current loop.~~ SESE: deal with the case where outer loops are moved into the current loop. Nov 1, 2018

bgogul requested review from mhong and lattner November 1, 2018 19:11

mhong approved these changes Nov 1, 2018

View reviewed changes

bgogul added 7 commits November 2, 2018 15:00

Minor refactoring of the SingleExitLoopTransformer code for reuse.

1d77d8b

Take care of loops that are moved from outside.

25a4866

Cleanup the canonicalization code.

abb5276

Added verifyLoop calls

c8aa190

Added a test for the movement of loops and loop cloning.

ee41be6

Add some detailed comments explaining some of the code.

4741ba2

Update comments and tests for better readability.

3fb0e2e

bgogul force-pushed the sese_move_outside_loops branch from ab5fee2 to 3fb0e2e Compare November 2, 2018 22:47

bgogul merged commit 61ed067 into apple:tensorflow Nov 2, 2018

bgogul deleted the sese_move_outside_loops branch November 2, 2018 23:42

swift-ci mentioned this pull request Nov 2, 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: deal with the case where outer loops are moved into the current loop. #20222

SESE: deal with the case where outer loops are moved into the current loop. #20222

bgogul commented Nov 1, 2018

bgogul commented Nov 1, 2018

mhong left a comment

mhong Nov 1, 2018

bgogul Nov 2, 2018

mhong Nov 1, 2018

bgogul Nov 2, 2018

mhong Nov 1, 2018

bgogul Nov 2, 2018

mhong Nov 1, 2018

bgogul Nov 2, 2018

mhong Nov 1, 2018

bgogul Nov 2, 2018

mhong Nov 1, 2018

bgogul Nov 2, 2018

mhong Nov 1, 2018

bgogul Nov 2, 2018

bgogul commented Nov 2, 2018

bgogul commented Nov 2, 2018

SESE: deal with the case where outer loops are moved into the current loop. #20222

SESE: deal with the case where outer loops are moved into the current loop. #20222

Conversation

bgogul commented Nov 1, 2018

bgogul commented Nov 1, 2018

mhong left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgogul commented Nov 2, 2018

bgogul commented Nov 2, 2018