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

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added a comment.

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?



================
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);
----------------
Shouldn't this loop skip the condition operand (operand 0) ?


================
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);
----------------
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.



================
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);
----------------
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).


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:9575
+      for (int I = 0, E = CI1->getNumOperands(); I < E; ++I) {
+        auto *Op0 = CI1->getOperand(I);
+        auto *Op1 = CI2->getOperand(Pred1 == Pred2 ? I : E - I - 1);
----------------
nit: Same as above: Op1, Op2 instead of Op0, Op1.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115268