[PATCH] D104598: [NFCI-ish][SimplifyCFGPass] Rework and generalize `ret` block tail-merging

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp:106
+
+    // Calls to experimental_deoptimize must be followed by a return
+    // of the value computed by experimental_deoptimize.
----------------
Oh boy, that's fun. This is the first I've learned of this verifier requirement. In the future, maybe there could be a way to generalize looking for musttail and looking for this intrinsic. I don't see a good way right now.


================
Comment at: llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp:117-118
+
+    // PHI nodes cannot have token type, so if the terminator has an operand
+    // with token type, we can not tail-merge this kind of function terminators.
+    if (any_of(Term->operands(),
----------------
See, tokens are totally straightforward compared to the deoptimize intrinsic. All the features that I've helped design are totally clear and straightforward. =P Or, at least I've tried to do my best, anyway.


================
Comment at: llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp:128-132
+      BasicBlock::iterator I(Term);
       --I;
       // Skip over debug info.
       while (isa<DbgInfoIntrinsic>(I) && I != BB.begin())
         --I;
----------------
Can this be `Term->getPrevNonDebugInstruction`?


================
Comment at: llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp:149
+    BasicBlock *CanonicalBB = nullptr;
+    SmallVector<PHINode *, 1> Ops;
+
----------------
Maybe `Phis` is a better name?


================
Comment at: llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp:181
+        // terminator.
+        auto *CanonicalTerm = Term->clone();
+        CanonicalBB->getInstList().push_back(CanonicalTerm);
----------------
In the end, the new instructions should carry the merged source location from the original instructions. You probably want to use `DILocation::getMergedLocations` for that. This is also worth testing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104598