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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added a comment.

In D115268#3178390 <https://reviews.llvm.org/D115268#3178390>, @vporpo wrote:

> It is probably worth splitting the patch, separating the fix for the crash from the improvements. It would also be nice to have a few small tests that check for these improvements to avoid regressions in the future. What do you think?

The fix alone causes test regressions.



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:9540
+      // Compare operands.
+      for (int I = 0, E = CI1->getNumOperands(); I < E; ++I) {
+        auto *Op0 = CI1->getOperand(Pred1 <= Pred2 ? I : E - I - 1);
----------------
vporpo wrote:
> Shouldn't this loop skip the condition operand (operand 0) ?
It is not for SelectInst but for CmpInst, it has only 2 operands and constant predicate


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:9541
+      for (int I = 0, E = CI1->getNumOperands(); I < E; ++I) {
+        auto *Op0 = CI1->getOperand(Pred1 <= Pred2 ? I : E - I - 1);
+        auto *Op1 = CI2->getOperand(Pred1 >= Pred2 ? I : E - I - 1);
----------------
vporpo wrote:
> If I understand correctly, at this point the predicates are either equal or opposites. You comparing them like `Pred1 <= Pred2` and `Pred1 >= Pred2`, which I think is equivalent to something like:
> ```
> bool PredsAreOpposite = Pred1 != Pred2;
> for (int I = 0, E = CI1->getNumOperands(); I < E; ++I) {
>   auto *Op0 = CI1->getOperand(I);
>   auto *Op1 = CI2->getOperand(!PredAreOpposite ? I : E - I - 1);
> ```
> I think this is what you did in line 9576 below.
> 
Yes, but in this form, it does not meet the criterion again for some cases, tried it. The provided form provides strict weak ordering.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:9574
+      // Compare operands.
+      for (int I = 0, E = CI1->getNumOperands(); I < E; ++I) {
+        auto *Op0 = CI1->getOperand(I);
----------------
vporpo wrote:
> Perhaps it is worth moving the operand comparison code in a separate function as it shares a lot with the operand comparison code in `CompareSorter()` above. It could return an enum like LT, GT, EQ (corresponding to return true, return false and continue).
No, the logic is different


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115268