[PATCH] Select Elimination in InstCombine

[Chandler Carruth]

(most of the other reviewers are doing a great job on this patch, so I've only made a couple of specific comments... will leave the rest of the review to them)

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2431
@@ +2430,3 @@
+//
+bool InstCombiner::dominatesAllUses(SelectInst *SI, ICmpInst &ICmp,
+                                    BasicBlock *D) {
----------------
joey wrote:
> I'm wondering if this name is too general, or if the parameter types are too strict.
Agreed. I Think the name is good, but it should be made a very generic property of an instruction.

I actually wonder whether this should be a method on DominatorTree itself.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:95
@@ -94,1 +94,3 @@
   AU.addRequired<TargetLibraryInfo>();
+  AU.addRequired<DominatorTreeWrapperPass>();
+  AU.addPreserved<DominatorTreeWrapperPass>();
----------------
echristo wrote:
> Probably don't need to have it as requires, but optional? i.e. if it doesn't exist then just don't do the transform in the way you've got?
I'm actually OK changing InstCombine so that it always requires dominator trees, but I think that should be a separate change.

I would add it as optional here, and then I would submit a follow-up patch to switch it over so we don't hold up the logic here on the debate about how often to require the analysis. Then the patch that makes it required can discuss the relevant concern by citing compile-item benchmarks showing it's not too expensive. =]

http://reviews.llvm.org/D5258