[PATCH] D19452: [instcombine] recognize three way comparison idioms

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2173
@@ +2172,3 @@
+  case Instruction::Select:
+    // match a select chain which produces one of three values based on whether
+    // the LHS is less than, equal to, or greater than RHS respectively.
----------------
Start with uppercase.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2175
@@ +2174,3 @@
+    // the LHS is less than, equal to, or greater than RHS respectively.
+    auto matchThreeWaySignedCompare = [](SelectInst *SI, Value *&LHS,
+                                         Value*& RHS, ConstantInt *&Less,
----------------
I'd put this in `InstCombineInternal.h`, that'll make it more likely that this gets used in other parts of IC.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2183
@@ +2182,3 @@
+      // select i1 (a == b), i32 0, i32 (select i1 (a < b), i32 -1, i32 1),
+      // except -1, 0, 1 are placeholders for any three constants
+      ICmpInst::Predicate PredA, PredB;
----------------
Can you please replace `-1`, `0` and `1` with `Less`, `Equal`, and `Greater` in the comments, so that is is more in tune with the code.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2191
@@ +2190,3 @@
+                         m_ConstantInt(Less), m_ConstantInt(Greater)))) {
+        if (PredA == ICmpInst::ICMP_EQ &&
+            PredB == ICmpInst::ICMP_SLT)
----------------
Minor, but I'd put the check on `PredA` right after you matched it, and the check on `PredB` after you matched it; since they're logically part of matching operation.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2210
@@ +2209,3 @@
+
+      bool TrueWhenLessThan =
+        ConstantExpr::getCompare(ICI.getPredicate(), C1, RHS)->isAllOnesValue();
----------------
I'd avoid the explicit combinatorial logic, but instead have:

```
  Value *Cond = getConstant(false);
  if (TrueWhenLessThan)
    Cond = CreateOr(Cond, CreateICmp(SLT, OrigLHS, OrigRHS))
  if (TrueWhenEqual)
    Cond = CreateOr(Cond, CreateICmp(EQ, OrigLHS, OrigRHS))
  if (TrueWhenGreater)
    Cond = CreateOr(Cond, CreateICmp(SGT, OrigLHS, OrigRHS))
```

And leave the rest of instcombine to combine, e.g., `a s< b || a == b`
into `a s<= b`.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2238
@@ +2237,3 @@
+
+    // TODO: This could obvious be extended to handle three way unsigned
+    // compare idioms as well.  Also, fcmp idioms?
----------------
s/obvious/obviously/ or drop "obvious"


http://reviews.llvm.org/D19452