[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:9490-9491
+                       function_ref<bool(Instruction *)> IsDeleted) {
+  if (V == V2)
+    return IsCompatibility;
+  auto *CI1 = cast<CmpInst>(V);
----------------
Since this is only used in `AreCompatibleCompares()` I think it makes sense to move it there. Do we expect to have identical values in CompareSorter() ?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:9498
+      CI2->getOperand(0)->getType()->getTypeID())
+    return !IsCompatibility;
+  if (CI1->getOperand(0)->getType()->getTypeID() >
----------------
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.



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:9532
+  }
+  return IsCompatibility;
+}
----------------
This is the value we return if we could not find a definite less-than or greater-than value. So I think it would make sense to use either a separate template variable for this, or a constant, something like `DefaultRetVal`:
```
template <bool RetValOnLessThan, bool DefaultRetVal = !RetValOnLessThan>
static bool compareCmp(Value *V, Value *V2, ...
```
or:
```
constexpr const bool DefaultRetVal = !RetValOnLessThan;
```


In this way `CompareSorter()` would call this function as:
```
return compareCmp</*RetValOnLessThan=*/true>(V, V2, ...);
```
and `AreCompatibleCompares()` would call this like:
```
return compareCmp</*RetValOnLessThan=*/false>(V1, V2,...);
```
What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115268