[PATCH] Expand SimplifyCFG to convert certain simple switches to selects

Philip Reames listmail at philipreames.com
Mon Jun 23 17:40:46 PDT 2014


I'm running out of useful comments.  :)  I don't have commit rights, so I can't give you an official LGTM, but what I see looks reasonable.  

Side note: this patch is getting really long.  To make review easier, a set of smaller changes would be a good idea.  Particularly since you have several independent changes which build on each other here.  

Also, since I'm not sure it was clear before, my wondering about other possible optimizations are _not_ things you need to fix before submission.  They might be TODOs to add or suggestions for future work, but unless they lead you to what you consider as a bug, you do not need to purse them.  I worry I may have led you slightly astray on that.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:184

----------------
This looks like an entirely new optimization rather than moving code around?  If so, do you mind separating this as a separate patch?  Most of the rest seems fairly solid, this needs a bit more work.

I had trouble tracing through the logic and assumptions here.  Example?  (Feel free to post a different review.)

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:236

----------------
To make your example clearer, you might vertically separate the two ands.  I had missed the &= usage on the left in first read through.

http://reviews.llvm.org/D4219






More information about the llvm-commits mailing list