[PATCH] D28907: [SLP] Fix for PR30787: Failure to beneficially vectorize 'copyable' elements in integer binary ops.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:334
 
-  bool isOpcodeOrAlt(Instruction *I) const {
-    unsigned CheckedOpcode = I->getOpcode();
-    return getOpcode() == CheckedOpcode || getAltOpcode() == CheckedOpcode;
+struct InstructionOrPseudo {
+
----------------
Turn this to a class. Also, not enough comments, the design of the structure also should be improved. Currently, it is too complex. Maybe base class + templates will help.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:394
+static bool allConstant(ArrayRef<InstructionOrPseudo *> IL) {
+  for (auto *i : IL)
+    if (!isa<Constant>(i->Op))
----------------
Please, follow the coding standard in the new code. Also, some of the functions can be replaced by some standard functions, just like in this case it can be replaced by `llvm::all_of`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:410
+/// false otherwise.
+static bool allSameBlock(ArrayRef<InstructionOrPseudo *> IL) {
+  if (!IL[0]->OpInst)
----------------
I think, the pseudo instruction is always can be considered as the one with the same block, because you can put it easily into the current block


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:427
 /// could be vectorized even if its structure is diverse.
 static InstructionsState getSameOpcode(ArrayRef<Value *> VL,
                                        unsigned BaseIndex = 0) {
----------------
I think this function also should accept `ArrayRef<InstructionOrPseudo *>` as an input param


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:801
+                                      ArrayRef<InstructionOrPseudo *> IL,
                                       SmallVectorImpl<Value *> &Left,
                                       SmallVectorImpl<Value *> &Right);
----------------
Left and Right must be `SmallVectorImpl<InstructionOrPseudo *> &`?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:845
     SmallVector<int, 1> UserTreeIndices;
+
   };
----------------
Restore the original


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:884
 
-  TreeEntry *getTreeEntry(Value *V) {
-    auto I = ScalarToTreeEntry.find(V);
-    if (I != ScalarToTreeEntry.end())
-      return &VectorizableTree[I->second];
+  TreeEntry *getTreeEntry(Instruction *I) {
+    if (!I)
----------------
Also, seems to me this must be an `InstructionOrPseudoOp`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1012
+
+    void init(int BlockSchedulingRegionID, ScheduleData *OpOrigin,
+              Value *OpParent, unsigned OpCode) {
----------------
Why do you need this new function? No comments and explanation.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1076
+        os << "/ ";
+        if (isPseudo())
+          os << "*";
----------------
Pseudo instruction must be just ignored?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1188
 
-    ScheduleData *getScheduleData(Value *V) {
-      ScheduleData *SD = ScheduleDataMap[V];
+    ScheduleData *getScheduleData(Instruction *I) {
+      ScheduleData *SD = InstScheduleDataMap[I];
----------------
Again, I think it must be `InstructionOrPseudoOp`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1264
 
-    void doForAllOpcodes(Value *V,
+    void doForAllOpcodes(Instruction *I,
                          function_ref<void(ScheduleData *SD)> Action) {
----------------
Again, `InstructionOrPseudoOp`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1324
 
+    /// Reorder bundles from PseudoScheduleData data after scheduling,
+    /// if an Instruction is present in PseudoScheduleData that means this
----------------
If you implement `InstructionOrPseudoOp` correctly, this reorder stuff should not be required, I think


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1345
     /// Note that the mapping survives during all vectorization iterations, i.e.
-    /// ScheduleData structures are recycled.
-    DenseMap<Value *, ScheduleData *> ScheduleDataMap;
+    /// InstScheduleData structures are recycled.
+    DenseMap<Instruction *, ScheduleData *> InstScheduleDataMap;
----------------
Why you cannot store everything in a single map?


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

https://reviews.llvm.org/D28907