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

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

Since this is off by default, it seems the changes to existing test files are unrelated?

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



================
Comment at: lib/Transforms/Scalar/SimplifyCFGPass.cpp:185
+                                    unsigned PhiSizeEstimate) {
+  // Do one pass to check if this is transformation is possible. Skip over phi
+  // nodes. They will be checked when they are used within the block, or they
----------------
nit: one "is" too many


================
Comment at: lib/Transforms/Scalar/SimplifyCFGPass.cpp:191
+  auto I2 = BB2->getFirstNonPHIOrDbg()->getIterator(),
+       E2 = BB2->end();
+  for (; I1 != E1 && I2 != E2;
----------------
No big deal, but I'd spell out auto for E1 and E2 too, to make the declarations look more conventional. It's the same number of characters anyway :-)


================
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.
----------------
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?


================
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())
----------------
Is there a reason for doing I1++ here instead of in the for-loop?


================
Comment at: lib/Transforms/Scalar/SimplifyCFGPass.cpp:255
+  // In a second pass, add phis for constant operands that differ and remove
+  // llvm.dbg.value intrinsics. Leave the debug locations of the BB1, which is
+  I1 = BB1->getFirstNonPHI()->getIterator();
----------------
nit: "which is" ... what? :-)


================
Comment at: lib/Transforms/Scalar/SimplifyCFGPass.cpp:361
+  if (Buckets.empty())
+    return Changed;
+
----------------
Since there's nothing that could have changed yet, maybe just return false here and declare Changed further down?


================
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) {
----------------
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?


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


https://reviews.llvm.org/D29428