[PATCH] D15679: [FIX] Schedule generation PR25879

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Johannes,

thanks for working on this. I have some two in-line comments.

Tobias


================
Comment at: lib/Analysis/ScopInfo.cpp:3472
@@ -3439,1 +3471,3 @@
+      LoopStack.push_back(L);
+    }
 
----------------
This piece of code looks a little complex. At the very least it requires some in-source comments.

>From my point of view, this code would possibly be easier to understand if we find a way to somehow (more explicitly) walk down the loop tree and just do the ReversePostOrderTraversal to process the statements within the loop bodies. We had such a tree walk already earlier, but it was removed when you generalized the schedule construction. Would adding back such a tree walk be difficult due to the generality of the CFGs we allow?

One of the reasons this might be difficult is if we aim to support irreducible control flow (http://llvm.org/PR25909). Due to LoopInfo working with natural loops which can not be formed from irregular control flow, we do not correctly model irregular control flow before and after this patch and I believe we should also not try to do so.  Do you have any intentions to support such control flow with your patch?

================
Comment at: test/ScopInfo/wrong_schedule.ll:66
@@ +65,3 @@
+return:                                           ; preds = %_eofout, %sw.epilog, %if.end.6
+  ret void
+}
----------------
Could you drop the control flow outside the scop? Most of it seems unnecessary to reproduce the test case and makes it harder to see the actual issue when looking at the IR itself.


http://reviews.llvm.org/D15679