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

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 3 inline comments as done.
rnk added a comment.

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.

> Can you comment on how this relates to https://reviews.llvm.org/D29153 ?

It's basically the same optimization, except more aggressive and on the IR-level, and I think we should do both. We duplicate a fair amount of CFG updating logic between MI and IR, and this is good because MI knows how to split the critical edges that this pass introduces with these phis. It also knows how to tail duplicate. See the X86 codegen test that shows that we generate good code for glibc's __assert_fail calls, but we still tail duplicate when the shared tail is too small.



================
Comment at: lib/Transforms/Scalar/SimplifyCFGPass.cpp:234
+
+  // Delete dead phis in BB1. The remaining phis will be updated when we process
+  // their uses in this block.
----------------
hans wrote:
> We haven't introduced any phis yet though, so this is just cleaning up to avoid left-over phis from previous passes that would get in the way of this transformation?
We modify the CFG below, but we only update phis with uses. Phis without uses (dead phis) need to be removed so that the IR remains valid.


================
Comment at: lib/Transforms/Scalar/SimplifyCFGPass.cpp:237
+  for (I1 = BB1->begin(); I1 != E1;) {
+    if (auto *Phi = dyn_cast<PHINode>(&*I1++)) {
+      if (Phi->use_empty())
----------------
hans wrote:
> Is there a reason for doing I1++ here instead of in the for-loop?
eraseFromParent would invalidate I1.


================
Comment at: lib/Transforms/Scalar/SimplifyCFGPass.cpp:365
+  // with it, leaving behind those which we fail to merge. Repeat the process on
+  // the remaining blocks until it is empty.
+  for (auto &HashBucket : Buckets) {
----------------
hans wrote:
> I found this comment a little confusing. Isn't it more like, for each bucket, attempt to merge each block in the bucket with the first, canonical, block?
reworded


================
Comment at: lib/Transforms/Scalar/SimplifyCFGPass.cpp:369
+    SmallPtrSet<BasicBlock *, 4> MergedBBs;
+    while (Bucket.size() > 1) {
+      DEBUG({
----------------
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?


https://reviews.llvm.org/D29428