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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev 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() >
----------------
vporpo wrote:
> 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. 
Do not treat this flag as a flag for the return value, treat it as a flag for the logic of the function. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115268