[PATCH] D85818: [UnifyFunctionExitNodes] Fix Modified status for unreachable blocks

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/UnifyFunctionExitNodes.cpp:47-49
 // new basic block.  The singular exit node is returned.
 //
 // If there are no return stmts in the Function, a null pointer is returned.
----------------
Slightly unrelated, but this looks wrong.

I assume it is describing what this function is returning (not what the singular exit node for the function being compiled is returning). But either way it is incorrect.

As this is an ordinary runOnFunction method I don't think it need to say anything special about what it returns (or it should say whatever other runOnFunction methods are saying).


================
Comment at: llvm/lib/Transforms/Utils/UnifyFunctionExitNodes.cpp:55
   //
   std::vector<BasicBlock*> ReturningBlocks;
   std::vector<BasicBlock*> UnreachableBlocks;
----------------
One idea (a bit larger refactoring) is to split this function by adding two new helpers, so the code in runOnFunction would look like:

```
{
  bool Changed = false;
  Changed |= unifyUnreachableBlocks();
  Changed |= unifyReturnBlocks();
  return Changed;
}
```
That would be nice since there doesn't seem to really be any dependency between the handling of unreachable and return blocks. And you wouldn't need to care about Changed status from one analysis inside the other.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85818