[PATCH] D115268: [SLP]Fix comparator for cmp instruction vectorization.

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:9498
+      CI2->getOperand(0)->getType()->getTypeID())
+    return !IsCompatibility;
+  if (CI1->getOperand(0)->getType()->getTypeID() >
----------------
ABataev wrote:
> vporpo wrote:
> > I think this value represents what we return when we find a definite less-than. So a better name for it would be something like: `RetValOnLessThan`. We should also probably set a default value for it `= true`, since this is what the comparator would return by default.
> > 
> I have a different idea behind `IsCompatibility` flag. If it is true, we check for compatible operations, if `false` - check for the ordering. I prefer to have a single flag rather than having several flags, which may affect the whole logic independently. This is a potential source of bugs.
But this does not explain why the other checks are not returning values based on `IsCompatibility`. I mean, isn't `if (BasePred1 > BasePred2)` also checking for compatibility? Why is it not returning a value like `!IsCompatibility` ?

What I am trying to say is that we should try to be more explicit about what is the purpose of `IsCompatibility`, otherwise this code is a bit cryptic. My understanding is that it is used for two separate things:
1. the return value of when we find a definite less-than, and
2. the default return value if we could not find a definite less-than or greater-than.
We could still use a single flag for those, but I don't think using two is too bad either. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115268