[PATCH] Select Elimination in InstCombine

[hfinkel at anl.gov]

================
Comment at: lib/Transforms/InstCombine/InstCombine.h:247
@@ -244,2 +246,3 @@
   Instruction *visitInstruction(Instruction &I) { return nullptr; }
+  bool dominatesAllUses(const Instruction *DI, const Instruction *UI, const BasicBlock *DB) const;
 
----------------
80 cols.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2428
@@ -2427,1 +2427,3 @@
 
+/// \brief Check that one use is in the same block as the defintion and all
+/// other uses are in blocks dominated by a given block
----------------
This is not quite right, the function actually asserts if the definition and the provided use are in different blocks (it does not check this condition).

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2458
@@ +2457,3 @@
+  const BasicBlock *BB = SI->getParent();
+  if (!BB)
+    return false;
----------------
Can this really be called on un-inserted instructions?

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2461
@@ +2460,3 @@
+  auto *BI = dyn_cast_or_null<BranchInst>(BB->getTerminator());
+  if (!BI || BI->getNumSuccessors() != 2)
+    return false;
----------------
Or on instructions in incomplete blocks?


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2975
@@ +2974,3 @@
+                ReplaceWithOpd = 1;
+            if (ConstantInt *CI = dyn_cast_or_null<ConstantInt>(Op1))
+              // When the constant operand of the select and cmp
----------------
Make this an else if?

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2988
@@ +2987,3 @@
+                ReplaceWithOpd = 2;
+            if (ReplaceWithOpd && I.isEquality()) {
+              // Replace select with operand on else path for EQ compares.
----------------
As we only actually perform the transform if I.isEquality(), can you host that part of the check up to be with the isChainSelectCmpBranch(SI) check?

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:100
@@ -98,1 +99,3 @@
   AU.addRequired<TargetLibraryInfo>();
+  AU.addRequired<DominatorTreeWrapperPass>();
+  AU.addPreserved<DominatorTreeWrapperPass>();
----------------
Are you going to separate this into a follow-up patch, or are you planning to provide compile-time measurements to before committing this one?

http://reviews.llvm.org/D5258