[PATCH] Select Elimination in InstCombine

[Chandler Carruth]

I still think that we need to confirm there is not a compile time regression from requiring dom trees here...

Also, some code comments below. I think this code needs to be refactored a bit here. =/ It's already gotten pretty gnarly and this pushes it further.

Thanks for working on all of this though. I really like the result.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2428-2431
@@ -2427,1 +2427,6 @@
 
+//
+// true when \param UI is the only use of \param DI in the parent block and
+// all other uses of \param DI are in blocks dominated by \param DB.
+//
+bool InstCombiner::dominatesAllUses(const Instruction *DI, const Instruction *UI,
----------------
Please follow either the specific coding standards for doxygen comments or match the surrounding code. The function above has a pretty good example.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2436-2437
@@ +2435,4 @@
+         "definition and use must be in the same block");
+  for (const Use &U : DI->uses()) {
+    auto *User = cast<Instruction>(U.getUser());
+    if (User != UI && !(DT && DT->dominates(DB, User->getParent())))
----------------
If all you need is the User, iterate over UI->users()?

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2942
@@ +2941,3 @@
+
+        bool Transform = false;
+        SelectInst *SI = cast<SelectInst>(LHSI);
----------------
This whole large new block is pretty indicative that this needs to be refactored. I think you should extract a method for the entire transform guarded by this boolean (in a separate patch, should be super simple refactoring).

The existence of the boolean is a really clear sign that this will help -- you can use early exit rather than a boolean to guard falling through to the transform. It will reduce indentation and make everything much more readable.

http://reviews.llvm.org/D5258