[PATCH] Select Elimination in InstCombine

[Eric Christopher]

A few nits in line, one question in line, and one more thought: Can't this replace a few of our other queries for instructions with dominator tree lookups? Is that a follow on patch or?

Thanks!

-eric

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2429
@@ +2428,3 @@
+//
+// true when \param SI dominates all of its uses or \param ICmp.
+//
----------------
s/or/of

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2433
@@ +2432,3 @@
+                                    BasicBlock *D) {
+  bool dominates_all_uses = true;
+  for (auto I = SI->use_begin(), E = SI->use_end(); I != E; ++I) {
----------------
Local variable naming style.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2945
@@ +2944,3 @@
+
+        bool transform = false;
+        SelectInst *SI = cast<SelectInst>(LHSI);
----------------
Local variable naming style.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2975
@@ +2974,3 @@
+            if (ReplaceWithOpd && IsEQorNECompare) {
+              // replace select with operand on else path for EQ compares.
+              // replace select with operand on then path for NE compares.
----------------
Nit: Sentences begin with capital letters.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2977
@@ +2976,3 @@
+              // replace select with operand on then path for NE compares.
+              int Path = I.getPredicate() == ICmpInst::ICMP_EQ ? 1 : 0;
+              if (InstCombiner::dominatesAllUses(
----------------
Isn't this just a boolean then?

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:95
@@ -94,1 +94,3 @@
   AU.addRequired<TargetLibraryInfo>();
+  AU.addRequired<DominatorTreeWrapperPass>();
+  AU.addPreserved<DominatorTreeWrapperPass>();
----------------
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?

http://reviews.llvm.org/D5258