[PATCH] D66607: [InstCombine] matchThreeWayIntCompare(): commutativity awareness

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.

Thank you for taking a look!



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2575-2576
+      // x sgt C-1  <-->  x sge C  <-->  not(x slt C)
+      auto FlippedStrictness =
+          getFlippedStrictnessPredicateAndConstant(PredB, cast<Constant>(RHS2));
+      if (!FlippedStrictness)
----------------
spatel wrote:
> I had not seen this API before. The header comment doesn't make sense:
>   For predicate of kind "is X or equal to 0"...
> 
> (what does zero constant have to do with this?)
> 
> ...and I don't like the name either, but that's independent of this patch. :)
I added `getFlippedStrictnessPredicateAndConstant()` in rL368820.
I'm open for the suggestions on the name, it mirrors the fact that
there's `CmpInst::getFlippedStrictnessPredicate()`, which it uses,
but it also takes care of the constant, so i found the name quite fitting.

I'm also unsure to what header comment you are referring to, can you give a link?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2588-2590
+  if (MatchPat0())
     return true;
   return false;
----------------
spatel wrote:
> Simplify?
>   return MatchPat0();
> But then, do we need the lambda at all?
I'm mimicking the original pattern (LHS of the diff)
The idea is - is this the only pattern that we could possibly have?
If no, then the lambda makes sense.
But i could inline it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66607/new/

https://reviews.llvm.org/D66607