[PATCH] Select Elimination in InstCombine

[Gerolf Hoflehner]

================
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;
 
----------------
hfinkel wrote:
> 80 cols.
ok

================
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
----------------
hfinkel wrote:
> 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).
the assertion actually checks that the use + def are in the *same* block

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

================
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;
----------------
hfinkel wrote:
> Or on instructions in incomplete blocks?
> 
dito.

================
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
----------------
hfinkel wrote:
> Make this an else if?
Part of refactoring.

================
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.
----------------
hfinkel wrote:
> 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?
Yes, thanks.

http://reviews.llvm.org/D5258