[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:9540
+      // Compare operands.
+      for (int I = 0, E = CI1->getNumOperands(); I < E; ++I) {
+        auto *Op0 = CI1->getOperand(Pred1 <= Pred2 ? I : E - I - 1);
----------------
ABataev wrote:
> 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
Oops my bad. I think I got confused by the checks in line 9523. They are checking operand 0 only, and they take place before the rest of the operand comparisons. 
Should these checks be incorporated into the new operand checks instead of having them as separate checks before the predicate comparisons?


================
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);
----------------
ABataev wrote:
> ABataev wrote:
> > 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.
> It does not work for these 2 cmp instructions: `icmp slt 0, x` and `icmp sgt x, 0` (`x` is an instruction). Compare returns true for compare(v1, v2) and compare(v2, v1) and thus does not meet the requirement for the weak strict ordering.
That makes sense. Anyway, I would still factor out the comparison out of the loop and use a variable, just to make it easier to read, something like:
```
bool SwapOperands1 = Pred1 < Pred2;
bool SwapOperands2 = Pred2 < Pred1;
for (int I = 0, E = CI1->getNumOperands(); I < E; ++I) {
  auto *Op1 = CI1->getOperand(SwapOperands1 ? I : E - I - 1);
  auto *Op2 = CI2->getOperand(SwapOperands2 ? I : E - I - 1);
```
but I don't feel too strongly about this.



================
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);
----------------
ABataev wrote:
> 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
My main concern is that if we make changes in the logic of `CompareSorter()` we should also make changes here too, and if we don't I think the compare instructions will be silently skipped. Perhaps it is worth mentioning this in a comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115268