[PATCH] D11481: [LoopUnswitch] Improve loop unswitch pass to find trivial unswitch conditions more effectively

[Bjarke Hammersholt Roune]

broune added inline comments.

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:752
@@ -751,3 +751,3 @@
 /// always unswitch trivial condition.
 ///
 bool LoopUnswitch::TryTrivialLoopUnswitch(bool &Changed) {
----------------
This comment is correct if we consider the header to extend across several basic blocks, as you do later on in a comment. I suspect that that's not a standard definition of the loop header (correct me if I'm wrong?), making this comment tricky to understand correctly. I think it would be clearer to explain this in terms of the first non-constant branch in the loop. Then probably the "Header" variable should be renamed to something else, like BB. Though this is subjective, so if you prefer the current way, I'll LGTM that.

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:768
@@ +767,3 @@
+  // LCSSA form might also not be preserved after deleting branches. The
+  // following code keeps traversing loop header's successors until find
+  // the trivial condition candidate (condition that is not a constant).
----------------
until find -> until it finds

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:792
@@ +791,3 @@
+      // Header is extended to its successor if it has only one
+      // reachable successor
+      if (BI->isUnconditional()) {
----------------
This wording suggests that the header is changed in some way, though that's not the case. I'm not sure you need a comment here, but if you keep it, maybe word it as something like "For this function, we consider the header to extend to its successor if ..." and add a full stop at the end.

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:800
@@ +799,3 @@
+      } else {
+        // Find a trivial condition candidate (non-foldable conditional branch)
+        break;
----------------
Find -> Found. Add full stop at the end.

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:804
@@ +803,3 @@
+    } else {
+      // Find a trivial condition candidate (switch instruction)
+      break;
----------------
Nit: This is not necessarily a switch. I think it would be OK without a comment here.

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:808
@@ +807,3 @@
+
+    HeaderTerm = Header->getTerminator();
+  }
----------------
Nit: HeaderTerm is set twice and only used twice. I would use Header->getTerminator() directly for the two uses instead, as then there's one variable less to keep track of.


http://reviews.llvm.org/D11481