[PATCH] D29016: [LoopUnswitch] Do not freeze condition if hoisted branch is guaranteed to be reachable

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:627
+        if (!isGuaranteedToTransferExecutionToSuccessor(Inst)) {
+          AlwaysReachableTI = false;
+        }
----------------
`break;`
No need to keep going if we're just going to break the outer loop.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:698
                                                currentLoop, Changed);
+        // Determine whether we should freeze the condition.
         if (LoopCond &&
----------------
This comment seems to make more sense in line 681 (which already has a similar comment), since that's where you decide if you need to freeze the condition (and set `NeedFreeze`).


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:978
+  // If there exists an instruction I which is (1) between the beginning of
+  // the loop preheader and the branch to unswitch, and (1) not guaranteed to 
+  // transfer execution to successor, then we need to freeze the condition.
----------------
`(2)` I guess :-)


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:1000
+        NeedFreeze |= !isGuaranteedToTransferExecutionToSuccessor(&I);
+      }
+    }
----------------
Minor nit: Maybe this instead of if + `|=`?
`NeedFreeze = NeedFreeze || !isGuaranteedToTransferExecutionToSuccessor(&I);`


https://reviews.llvm.org/D29016