[PATCH] D14122: [SimplifyCFG] Trim duplicate basic blocks in switch cases

Philip Reames via llvm-commits llvm-commits at lists.llvm.org
Wed Oct 28 21:39:14 PDT 2015


If you decide to go forward with the prefix matching, SimplifyCFG is 
absolutely the right place.  I'm really bothered by the fact that target 
specific tests are sensitive to a target independent transform pass at 
all.  I didn't follow that part of the review, but any way we can fix 
that? Working around it further by introducing a new pass is definitely 
not something we should do.

Philip

On 10/28/2015 09:28 PM, Dylan McKay wrote:
> dylanmckay marked 2 inline comments as done.
> dylanmckay added a comment.
>
> Phillip's idea is a good one - it would be fairly easy to generalize this optimization so that it performes "prefix merging".
>
> Do you think the new optimization should still live in `SimplifyCFG`? One problem with keeping it in `SimplifyCFG` is that block merging will interfere with tests even worse than with this patch (which is why the current patch adds a command line option to disable merging), causing them to wonder "where has my basic block gone?".
>
> If the patch were to be split into a separate pass, it would be quite a small pass which would be overkill. Perhaps it would be better to keep the optimization in `SimplifyCFG` and keep the command line option for testing purposes.
>
> Thoughts?
>
>
> ================
> Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3461
> @@ -3418,3 +3460,3 @@
>     BasicBlock::iterator I = Succ->begin();
>     while (PHINode *PHI = dyn_cast<PHINode>(I++)) {
>       int Idx = PHI->getBasicBlockIndex(BB);
> ----------------
> I understand this now - thanks for the explanation Sanjoy!
>
>
> http://reviews.llvm.org/D14122
>
>
>



More information about the llvm-commits mailing list