[PATCH] D101109: [SLP]Improve multinode analysis.

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1458
+              I->getParent() != Parent) {
+            if (NumOpsWithSameOpcodeParent == 0) {
+              NumOpsWithSameOpcodeParent = 1;
----------------
Are you using `NumOpsWithSameOpcodeParent == 0` as a check for the first iteration ? Shouldn't you be using `!OpcodeI` intsead ?

I find this code a bit hard to follow, because I can't tell which of the `if` conditions are for checking for the first iteration and which ones are part of the heuristic.  Should it be updating the `OpcodeI` and `Parent` only in the first iteration (like below), or should it be doing it whenever there is a mismatch?
```
if (auto *I = dyn_cast<Instruction>(OpData.V)) {
  // First iteration
  if (!OpcodeI) {
    OpcodeI = I;
    Parent = I->getParent();
  }
  // Mismatch
  if (!getSameOpcode({OpcodeI, I}).getOpcode() || I->getParent() != Parent)
    ++NumOpsWithSameOpcodeParent;
   else
     NumOpsWithSameOpcodeParent = std::min(NumOpsWithSameOpcodeParent-1, 0);
}
```
Perhaps peeling the first iteration might make the code easier to follow?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1459
+            if (NumOpsWithSameOpcodeParent == 0) {
+              NumOpsWithSameOpcodeParent = 1;
+              OpcodeI = I;
----------------
Why is `NumOpsWithSameOpcodeParent` set to 1 the first time a mismatch is found? Shouldn't it be set to 0 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101109