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

[Dinar Temirbulatov via Phabricator via llvm-commits]

dtemirbulatov marked 12 inline comments as done.
dtemirbulatov 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 {
+
----------------
ABataev wrote:
> 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.
I see this class as just one, BTW it is a container.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:410
+/// false otherwise.
+static bool allSameBlock(ArrayRef<InstructionOrPseudo *> IL) {
+  if (!IL[0]->OpInst)
----------------
ABataev wrote:
> 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
I have this functionality, but let us for now minimize functionality change here, I will follow this change right after that check-in.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:801
+                                      ArrayRef<InstructionOrPseudo *> IL,
                                       SmallVectorImpl<Value *> &Left,
                                       SmallVectorImpl<Value *> &Right);
----------------
ABataev wrote:
> Left and Right must be `SmallVectorImpl<InstructionOrPseudo *> &`?
That change further increases the size of this review, we can change that later.


================
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)
----------------
ABataev wrote:
> Also, seems to me this must be an `InstructionOrPseudoOp`
no, looks like "Instruction" here is more convenient.


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

https://reviews.llvm.org/D28907