[PATCH] D78277: [SimpleLoopUnswitch] Update DefaultExit condition to check unreachable is not empty.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:601
 
+  auto ArePhisLoopInvAndNotUnreachable = [&](BasicBlock *BBToCheck) {
+    bool isLoopExitAndPhisInvariant =
----------------
Rather than trying to name after the implementation, how about naming after the usage? `IsTriviallyUnswitchableExitBlock`?

I'd also use a reference here, but that's a pretty minor point.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:602-609
+    bool isLoopExitAndPhisInvariant =
+        !L.contains(BBToCheck) &&
+        areLoopExitPHIsLoopInvariant(L, *ParentBB, *BBToCheck);
+    auto *TI = BBToCheck->getTerminator();
+    bool isUnreachable = isa<UnreachableInst>(TI);
+    bool isNotEmptyUnreachable =
+        !isUnreachable || (isUnreachable && (&*BBToCheck->begin() != TI));
----------------
I'd rewrite this to use early exit rather than combining flags, and add some comments about the non-obvious part (IMO, the non-empty unreachable).

Something like:

```
if (L.contains(BBToCheck))
  return false;
if (!areLoopExitPHIsLoopInvariant(L, *ParentBB, *BBToCheck))
  return false;
....
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78277/new/

https://reviews.llvm.org/D78277