SESE: update LoopInfo incrementally when cloning a loop body with nested loops. #20167

bgogul · 2018-10-30T22:55:45Z

When a loop contains nested loops and is cloned, the LoopInfo becomes stale. e.g.,

while ... {    // loop 1
  foo(...)   
  while ... {}  // loop 2
  bar(...)
}

After cloning, we have the following:

foo() 
while ... {}    // loop 2'
bar()
while ... {     // loop 1
   foo()
   while ... {} // loop 2
   bar()
}

Note that we have a new top-level loop loop 2'. This PR makes sure that LoopInfo is incrementally updated for such cases.

bgogul · 2018-10-30T22:56:00Z

@swift-ci please test tensorflow

mhong

The code structure looks good from a quick scan.

But I lost some larger context -- can you please extend PR description by elaborating on what "preserve the nesting structure" means? It might be useful to motivate this PR with an example, and describe how this PR helps in that context.

mhong · 2018-10-31T02:42:49Z

test/TensorFlow/sese_loop_canonicalization.swift

+// the body of a loop during canonicalization.
+//expected-warning @+1 {{value implicitly copied to the host}}
+public func testLoopWithNestedLoopsRequiringCloning(
+	_ breakIndex:Int32, _ repetitions: Int32) -> Tensor<Int32> {


replace tab with space?

mhong · 2018-10-31T02:44:59Z

test/TensorFlow/sese_loop_canonicalization.swift

+// CHECK:         Loop at depth 3 containing: {{.*}}
+// CHECK:             Loop at depth 4 containing: {{.*}}
+
+// CHECK: --- XLA CFG Loops Before Canonicalize end


should we instead CHECK for // CHECK-LABEL: --- XLA CFG Loops After Canonicalize: ?

I believe CHECK-LABEL is better here. As far as I understand, CHECK-LABEL is used for breaking checks into logical blocks.

Right. My question is whether we want to check for After instead of Before Canonicalize end

Oops, it should After.

mhong · 2018-10-31T02:46:32Z

test/TensorFlow/sese_loop_canonicalization.swift

+// CHECK:     Loop at depth 2 containing: {{.*}}
+// CHECK:         Loop at depth 3 containing: {{.*}}
+
+//-- Check the structure of the SESE region. (Note the cloned doubly nested loop.)


this CFG below is very complex and I did not review it closely.

What are the key parts that we want to check and make sure we get right, and is it possible to simplify the test?

This is the simplest example that I could come up with.

Basically, we are checking a couple of things:

The LoopInfo is correct before and after the canonicalization. We only check the loop nesting structure though.

The clone loop body is structurally correct. I added a comment pointing to the loop that is being cloned in the test. Hope that improves the readability a bit?

mhong · 2018-10-31T02:48:00Z

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

+  /// fragment into the parent loop. If `startBlock` is the same as the header
+  /// of `loop`, we clone the entire loop including the back edge.  Otherwise,
+  /// we clone one iteration of the loop body without the back edge.
+  SILLoop *cloneLoop(SILLoopInfo *LI, SILLoop *loop, SILBasicBlock *startBlock) {


from the call site below, it seems startBlock is always header. Why do we need the other code path?

The other code path will be used in a subsequent PR that I am preparing.

@mhong has a valid point

In this use case, we are only cloning part of the loop. The header that is being passed at the call-site is the original header of the loop before canonicalization. After canonicalization, the loop will look like this:

new_header: cond_br stayInLoop header loop_exit header: ... br latch latch: ... br new_header exit: ...

We are only cloning the part of the loop reachable from header and the result would look something like this (primed names are the clones):

header': ... br latch' latch': ... br new_header new_header: cond_br stayInLoop header loop_exit header: ... br latch latch: ... br new_header exit: ...

In the other path, we will pass in new_header and thereby clone the entire loop. Will send out that PR shortly.

atrick · 2018-10-31T15:52:21Z

I have the same question. What does "preserve loop structure" mean? Does it simply mean that LoopInfo will be incrementally updating during cloning?

bgogul · 2018-10-31T16:17:29Z

I have the same question. What does "preserve loop structure" mean? Does it simply mean that LoopInfo will be incrementally updating during cloning?

Yes, LoopInfo is incrementally updated during cloning. I have changed the PR description as it is much better. :)

mhong

Thanks for the additional context/discussion!

mhong · 2018-10-31T18:02:11Z

lib/SILOptimizer/Mandatory/TFCanonicalizeCFG.cpp

@@ -189,6 +198,18 @@ namespace {
                                     SILBasicBlock *endBB);
    void processLoop(SILLoop *loop);
    void ensureSingleExitFromLoops();
+
+    // Dump top-level loop information for debugging purposes.
+    void dumpTopLevelLoopInfo(llvm::raw_ostream* outs, const char* stage) {


would it be useful to have a routine that dumps loop info for a specific loop? this way after transforming one loop, we can just dump out it's structure before and after.

We can already do that with loop->dump() or loop->print(...)?

would it be useful for debugging, if we dump the loop info out before and after the transformation, for each transformation step?

mhong · 2018-10-31T18:06:51Z

test/TensorFlow/sese_loop_canonicalization.swift

+// CHECK: Loop at depth 1 containing: {{.*}}
+// CHECK:     Loop at depth 2 containing: {{.*}}
+// CHECK:         Loop at depth 3 containing: {{.*}}
+// CHECK:     Loop at depth 2 containing: {{.*}}


is there another nested "Loop at depth 3 containing" that we should also CHECK here?

No. I think the confusion comes from the use of the term "cloning" here. I am actually referring to the unrolling the body of the loop at depth 2. When we nest the inner loop at depth 3 into the outer loop at depth 1, we get a loop of depth 2.

I have replaced uses of cloning with unrolling. Hope that clarifies things a bit.

I understand now. Thanks.

mhong · 2018-10-31T18:07:18Z

test/TensorFlow/sese_loop_canonicalization.swift

+//-- Check the structure of the SESE region. (Note the cloned loop.)
+// CHECK-LABEL: --- XLA CFG Canonicalize: {{.*}}LoopWithNestedLoopsRequiringCloning{{.*}}
+// CHECK: [sequence
+// CHECK:   <while Preheader: {{.*}}, Header: {{.*}}, exit: {{.*}}


does this correspond to "Loop at depth 2" above? I didn't find 3 levels of loop nesting in the CFG here.

See comment above.

mhong · 2018-10-31T18:08:08Z

test/TensorFlow/sese_loop_canonicalization.swift

+// CHECK:                 block {{.*}}
+// CHECK:                 block {{.*}}}]}
+// ---The body of this loop is cloned above---
+// CHECK:           <while Preheader: {{.*}}, Header: {{.*}}, exit: {{.*}}


should the indentation here match that of the line 58 above?

No, the indentation is correct. This loop is nested in the while-loop at line 58.

mhong · 2018-10-31T18:09:15Z

test/TensorFlow/sese_loop_canonicalization.swift

+// CHECK:     Loop at depth 2 containing: {{.*}}
+// CHECK:         Loop at depth 3 containing: {{.*}}
+// CHECK:             Loop at depth 4 containing: {{.*}}
+// CHECK:     Loop at depth 2 containing: {{.*}}


so we are cloning the loop at depth 2 -- consider adding a comment at the source code test case above, pointing out which loop this corresponds to.

Good idea. Done.

mhong · 2018-10-31T18:10:06Z

test/TensorFlow/sese_loop_canonicalization.swift

+//-- Check the structure of the SESE region. (Note the cloned doubly nested loop.)
+// CHECK-LABEL: --- XLA CFG Canonicalize: {{.*}}LoopWithDoublyNestedLoopsRequiringCloning{{.*}}
+// CHECK: [sequence
+// CHECK:   <while Preheader: {{.*}}, Header: {{.*}}, exit: {{.*}}


similar questions to above:

does this loop correspond to "loop at depth 2", not 1?

should line 180 be indented to match the indentation here?

See the related comments above.

… unrolling and cloning.

bgogul · 2018-11-01T18:51:26Z

Thanks for the comments, Ming. I think the source of confusion in interpreting tests is that I was using the term cloning instead of unrolling. Hope the new changes make it more readable.

I will go ahead and merge it so that I can send out the subsequent PRs.

bgogul requested review from mhong and lattner October 30, 2018 23:58

mhong reviewed Oct 31, 2018

View reviewed changes

bgogul added 3 commits October 31, 2018 04:13

Added debugging information about loop structure.

7b23263

When unrolling loops clone the loop hierarchy as well.

c870f71

Added a test to check loop structure is preserved.

9fc2410

bgogul changed the title ~~Preserve loop nesting structure when cloning loop body~~ SESE: update LoopInfo incrementally when cloning a loop body with nested loops. Oct 31, 2018

Added some comments to the example

226cde3

bgogul force-pushed the sese_preserve_loop_structure branch from 0224b15 to 226cde3 Compare October 31, 2018 16:27

mhong approved these changes Oct 31, 2018

View reviewed changes

Updated the comments in the test to reflect that we are talking about…

2640a75

… unrolling and cloning.

bgogul merged commit b7f2fda into swiftlang:tensorflow Nov 1, 2018

bgogul deleted the sese_preserve_loop_structure branch November 1, 2018 19:12

swift-ci mentioned this pull request Nov 2, 2018

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

Closed

SESE: update LoopInfo incrementally when cloning a loop body with nested loops. #20167

SESE: update LoopInfo incrementally when cloning a loop body with nested loops. #20167

Uh oh!

Conversation

bgogul commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bgogul commented Oct 30, 2018

Uh oh!

mhong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atrick commented Oct 31, 2018

Uh oh!

bgogul commented Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgogul commented Nov 1, 2018

Uh oh!

Uh oh!

bgogul commented Oct 30, 2018 •

edited

Loading

bgogul commented Oct 31, 2018 •

edited

Loading