[PATCH] D29428: [SimplifyCFG] Merge similar fatal error blocks ending in unreachable

[Hans Wennborg via Phabricator via llvm-commits]

hans accepted this revision.
hans added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D29428#664945, @rnk wrote:

> In https://reviews.llvm.org/D29428#664902, @hans wrote:
>
> > Since this is off by default, it seems the changes to existing test files are unrelated?
>
>
> Somewhat. The other test changes make them resilient to changing the default for -merge-noreturn-bbs. That seems like a useful improvement that I don't want to lose track of.


sgtm



================
Comment at: lib/Transforms/Scalar/SimplifyCFGPass.cpp:369
+    SmallPtrSet<BasicBlock *, 4> MergedBBs;
+    while (Bucket.size() > 1) {
+      DEBUG({
----------------
rnk wrote:
> hans wrote:
> > It's not clear to me why this needs multiple iterations. If merging a block the first time doesn't succeed, why would it work after others have been merged?
> I can't construct a test case for this because I hash pointer values, but imagine that we have two pairs of identical noreturn blocks, and they all hash to the same bucket. We pick one, iterate the other three, merge the one that matches, and remove them from the bucket. Then we repeat with the remaining two and merge them together.
> 
> We could test this scenario by adding a command line flag that makes every block hash to '0'. Do you think it's worth it?
Oh, I just didn't consider the hash collisions. This makes sense, and I don't think it's worth adding a flag to test it. Maybe it's possible to explain it in a comment though.


https://reviews.llvm.org/D29428