[PATCH] D127921: [FunctionPropertiesAnalysis] Generalize support for unreachable

[Kazu Hirata via Phabricator via llvm-commits]

kazu added a comment.

LGTM modulo cosmetic changes.  Your new algorithm looks pretty robust to me.  Thanks!



================
Comment at: llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:156-160
+  if (!isa<CallInst>(CB)) {
+    const auto &II = cast<InvokeInst>(CB);
+    const auto *UnwindDest = II.getUnwindDest();
+    Successors.insert(succ_begin(UnwindDest), succ_end(UnwindDest));
+  }
----------------
An instruction can never be `CallInst` and `InvokeInst` at the same time because `InvokeInst` and `CallInst` are siblings derived from `CallBase`.  Yo could shave one line with:

```
  if (const auto *II = dyn_cast<InvokeInst>(&CB)) {
    const auto *UnwindDest = II->getUnwindDest();
    Successors.insert(succ_begin(UnwindDest), succ_end(UnwindDest));
  }
```



================
Comment at: llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:230-232
+    if (I < DoNotIncludeSuccessorsMark)
+      continue;
+    Reinclude.insert(succ_begin(BB), succ_end(BB));
----------------
Could we avoid the negated name for clarity?  We can shorten the variable name and remove one line.

```
    if (I >= IncludeSuccessorsMark)
      Reinclude.insert(succ_begin(BB), succ_end(BB));
```


================
Comment at: llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:233
+    Reinclude.insert(succ_begin(BB), succ_end(BB));
+  }
+  // For exclusion, we don't need to exclude the set of BBs that were successors
----------------
May I suggest an empty line immediately after the loop?  We are starting a new code block here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127921