[PATCH] D34278: [InstCombine] Recognize and simplify three way comparison idioms

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2454
+                     m_ConstantInt(Less), m_ConstantInt(Greater)))) {
+    if (PredA == ICmpInst::ICMP_EQ && PredB == ICmpInst::ICMP_SLT)
+      return true;
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > We can check PredA right after we matched line 2450 to avoid further maching if we have a wrong comparison there.
> Maybe just another && instead of second if?
agreed.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2474
+                                 C1LessThan, C2Equal, C3GreaterThan)) {
+    assert(C1LessThan && C2Equal && C3GreaterThan && C);
+
----------------
mkazantsev wrote:
> Can we assert C on method entry?
yes. I'll move it.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2495
+    Value *Cond = Builder->getFalse();
+    if (TrueWhenLessThan)
+      Cond = Builder->CreateOr(Cond, Builder->CreateICmp(ICmpInst::ICMP_SLT, OrigLHS, OrigRHS));
----------------
mkazantsev wrote:
> Is it possible that more than one of {TrueWhenLessThan, TrueWhenEqual, TrueWhenGreaterThan} is true?
> 
> If no, please add an assertion on that and use "else if" below.
> If yes, should we not consider this when creating conditions?
Yes, more than one can be true at a time: this actually enumerates all the possible 8 combinations, because of the chaining of these ORs in `CreateOr`. We pass in the `Cond` as the parameter. 

(I actually liked this idea a lot: credit goes to Sanjoy in suggesting this in the original review).

Please see the comment above the code, should I perhaps clarify it more? 




https://reviews.llvm.org/D34278