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

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added a comment.

Perhaps the score changes could be split into a separate patch?



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1337
+      unsigned SameOpNumber = 0;
+      MapVector<unsigned, std::pair<unsigned, unsigned>> HashMap;
+      for (int I = getNumLanes(); I > 0; --I) {
----------------
Could you add a comment what these `unsigned` values are for? (or perhaps use a struct instead?). 
Could you also describe the the purpose of `HashMap` and what the keys and values are? 


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1338
+      MapVector<unsigned, std::pair<unsigned, unsigned>> HashMap;
+      for (int I = getNumLanes(); I > 0; --I) {
+        unsigned Lane = I - 1;
----------------
Is there any reasoning behind the iteration in reverse? If so could you please add a comment?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1379
+      /// parent.
+      unsigned SameCodeParentOps = 0;
+      /// Hash for the actual operands ordering.
----------------
NIT: I find `Code` a bit confusing, also perhaps there is no need to refer to `Parent` in the variable name?  Perhaps rename to something like `NumOpsWithSameOpcode`?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1380
+      unsigned SameCodeParentOps = 0;
+      /// Hash for the actual operands ordering.
+      unsigned Hash = 0;
----------------
Could you add a bit more text in the comment what the hashed is used for? I can see that it is used as a key in the `HashMap` above, but could you explain how it is being used?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1396-1397
       // the other by subtracting it from the total number of operands.
-      for (unsigned OpIdx = 0; OpIdx != NumOperands; ++OpIdx)
-        if (getData(OpIdx, Lane).APO)
+      // Operands with the same instruction opcode and parent are more
+      // profitable since we don't need to move them in many cases.
+      bool AllUndefs = true;
----------------
Could you elaborate a bit on this? If I understand correctly the more similar opcodes we can find, the easier it is to reorder them, therefore this can act as a tie-breaker when the NumOfAPOs is equal?



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1408
+        if (auto *I = dyn_cast<Instruction>(OpData.V)) {
+          if (!OpcodeI || !getSameOpcode({OpcodeI, I}).getOpcode() ||
+              I->getParent() != Parent) {
----------------
If I am not mistaken this code will count the consecutive operands with same opcode and BB. Is it because this is a good enough approximation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101109