[PATCH] Select Elimination in InstCombine

[Gerolf Hoflehner]

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

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2431
@@ +2430,3 @@
+//
+bool InstCombiner::dominatesAllUses(SelectInst *SI, ICmpInst &ICmp,
+                                    BasicBlock *D) {
----------------
chandlerc wrote:
> 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.
I generalized the signature to use instruction *. 

================
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) {
----------------
echristo wrote:
> Local variable naming style.
Fixed.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2452-2454
@@ +2451,5 @@
+    return false;
+  if (!isa<BranchInst>(BB->getTerminator()))
+    return false;
+  BranchInst *BI = cast<BranchInst>(BB->getTerminator());
+  if (BI->getNumSuccessors() != 2)
----------------
majnemer wrote:
> Why not perform a dyn_cast instead?
Right, thanks.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2457-2460
@@ +2456,6 @@
+    return false;
+  ICmpInst *IC = dyn_cast<ICmpInst>(BI->getCondition());
+  if (IC && IC->getOperand(0) != SI)
+    return false;
+  return true;
+}
----------------
majnemer wrote:
> This will return true if IC is null, did is this intensional?
Excellent catch, thanks!

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

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2955
@@ +2954,3 @@
+            // Code sequence is select - icmp.eq - br
+            int ReplaceWithOpd = 0;
+            if (Op2)
----------------
majnemer wrote:
> s/int/unsigned
ok, unsigned.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2957
@@ +2956,3 @@
+            if (Op2)
+              if (ConstantInt *CI = dyn_cast<ConstantInt>(Op2))
+                if (CI->getValue().getBoolValue())
----------------
joey wrote:
> I think you can use dyn_cast_or_null here to combine these two if statements, to reduce indentation.
nice suggestion, thanks.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2965
@@ +2964,3 @@
+            if (Op1)
+              if (ConstantInt *CI = dyn_cast<ConstantInt>(Op1))
+                if (CI->getValue().getBoolValue())
----------------
joey wrote:
> dyn_cast_or_null again.
dito

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2972-2973
@@ +2971,4 @@
+                  ReplaceWithOpd = 2;
+            bool IsEQorNECompare = I.getPredicate() == ICmpInst::ICMP_EQ ||
+                                   I.getPredicate() == ICmpInst::ICMP_NE;
+            if (ReplaceWithOpd && IsEQorNECompare) {
----------------
majnemer wrote:
> This is just llvm::ICmpInst::isEquality.
yes, thanks!

================
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.
----------------
echristo wrote:
> Nit: Sentences begin with capital letters.
Yop, fixed.

================
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(
----------------
echristo wrote:
> majnemer wrote:
> > echristo wrote:
> > > Isn't this just a boolean then?
> > I'd make this an unsigned, getSuccessor is asking which successor to take.  It just so happens that we are only interested in cases where there are two successors.
> Yes. Makes sense that way. Might be better off just grabbing the correct successor rather than mucking with the integer.
Addressed as suggested.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:95
@@ -94,1 +94,3 @@
   AU.addRequired<TargetLibraryInfo>();
+  AU.addRequired<DominatorTreeWrapperPass>();
+  AU.addPreserved<DominatorTreeWrapperPass>();
----------------
chandlerc wrote:
> 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. =]
We go for required. Open question is one or two commits.

================
Comment at: test/Transforms/InstCombine/select-cmp-br.ll:1
@@ +1,2 @@
+; Checks if select is replaced by or in select - cmp [eq|ne] - br sequence
+; RUN: opt -instcombine -S < %s | FileCheck %s
----------------
mcrosier wrote:
> Perhaps something like:
> ; Replace a 'select' with an 'or' in a 'select - cmp[eq|ne] - br' sequence when profitable.
Done.

================
Comment at: test/Transforms/InstCombine/select-cmp-br.ll:9
@@ +8,3 @@
+
+define void @foo(%C*) {
+entry:
----------------
mcrosier wrote:
> @test1
done.

================
Comment at: test/Transforms/InstCombine/select-cmp-br.ll:10
@@ +9,3 @@
+define void @foo(%C*) {
+entry:
+  %1 = getelementptr inbounds %C* %0, i64 0, i32 0, i32 0
----------------
mcrosier wrote:
> CHECK-LABEL: @test1
dito for this and the rest below.

http://reviews.llvm.org/D5258