[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:
> > 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. 
OK suppose somebody else wants to extend the comparator function to go even deeper through the operands and add more comparisons. How would `IsCompatibility` guide them in writing the code? They could as well treat it as a flag and do a multi-versioning implementation like: if (IsCompatibility) { do the logic for the compatibility comparison; } else { do the comparison logic; }. 

If instead we are more clear about how we expect the flag to be used and name it like `ReturnValOnLessThan`, then they would have no choice but to use it as expected.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115268