[PATCH] D59973: [SLP] Refactoring of the operand reordering code.

Alexey Bataev via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 4 06:53:03 PDT 2019


ABataev added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:714
+      OperandData(Value *V, bool APO, bool IsUsed)
+          : V(V), APO(APO), IsUsed(IsUsed) {};
+      /// The operand value.
----------------
Remove semicolon


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:803
+      ValueList OpVL;
+      for (auto &OpData : OpsVec[OpIdx])
+        OpVL.push_back(OpData.V);
----------------
`auto &`->`const OperandDataVec &` for better readability (it is not very easy to deduce the type)


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3063
+    // Get the operand at Idx and Lane.
+    auto &OpData = Ops.getData(Idx, Lane);
+    Value *Op = OpData.V;
----------------
`auto &` to real type


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3224
+// Perform operand reordering on the instructions in VL.
+void BoUpSLP::reorderInputsAccordingToOpcode(ArrayRef<Value *> VL,
+                                             SmallVectorImpl<Value *> &Left,
----------------
Maybу it would be better to encapsulate all this reordering in the `VLOperands` class rather than exposing some low-level transformators like `swap` etc.?


================
Comment at: test/Transforms/SLPVectorizer/X86/operandorder.ll:47
 ; CHECK-NEXT:    store <2 x double> [[TMP4]], <2 x double>* [[TMP5]], align 4
 ; CHECK-NEXT:    br i1 undef, label [[LP]], label [[EXT:%.*]]
 ; CHECK:       ext:
----------------
vporpo wrote:
> RKSimon wrote:
> > Hmm - these tests are called 'preserve_broadcast' and we're not preserving the broadcasts any more.... This might not be a bad thing but it'd be good to confirm (and possibly rename the tests if everything is good)
> I am not sure why we would prefer to build a broadcast instead of vectorizing the loads.
> @ABataev any idea about this?
I agree, that the vectorization of loads is better.


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

https://reviews.llvm.org/D59973





More information about the llvm-commits mailing list