[PATCH] D64162: Summary: [Attributor] Liveness analysis abstract attribute used to indicate which BasicBlocks are dead and can therefore be ignored.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

We need more tests, including but not limited to:

- Unreachable blocks.
- Dead blocks caused by something other than noreturn, e.g., infinite loops, recursion, undefined behavior etc. (these will be "FIXME" tests)
- An assumed noreturn call that is later not assumed noreturn anymore. (I'll work basic on no-return deduction now.)



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1103
+  // get new paths (reachable blocks).
+  bool HasNewPath = false;
+  for (BasicBlock *SuccBB : successors(BB)) {
----------------
I think any advance above, e.g., any new live instruction, should result in a `ChangeStatus::CHANGED` result from `updateImpl`.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1108
+    if (!ToBeExploredPaths.count(Inst)) {
+      ToBeExploredPaths.insert(Inst);
+      HasNewPath = true;
----------------
```
if (ToBeExploredPaths.insert(Inst).second)
 HasNewPath = true;
```


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1130
+    while (Size != ToBeExploredPaths.size())
+      explorePath(A, ToBeExploredPaths[Size++]);
+  }
----------------
You need to update the `NoReturnCalls` set. However, now you modify it while iterating, that is a problem.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1138
+  if (AssumedLiveBlocks.size() < F.size())
+    return ChangeStatus::UNCHANGED;
+
----------------
You can have new live blocks even if not all blocks are live.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64162