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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
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)
----------------
lebedev.ri wrote:
> 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?
I was looking at the existing/underlying function:
https://github.com/llvm-mirror/llvm/blob/master/include/llvm/IR/InstrTypes.h#L856

So my comment was only about stuff that is independent of this patch, so feel free to ignore.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2588-2590
+  if (MatchPat0())
     return true;
   return false;
----------------
lebedev.ri wrote:
> 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.
I'd go with the simpler form for now. If we end up adding code/patterns, then that would be the time to add code/flexibility.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66607