[PATCH] D26905: [SLP] Vectorize loads of consecutive memory accesses, accessed in non-consecutive (jumbled) way.

[Simon Pilgrim via llvm-commits]

RKSimon added a comment.

Some minor comments - mainly code style etc.



================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1021
+                                        const DataLayout &DL,
+                                        ScalarEvolution &SE) {
+  std::multimap<int, Value *> offValPair;
----------------
clang-format this


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1023
+  std::multimap<int, Value *> offValPair;
+  for (unsigned i = 0; i < VL.size(); i++) {
+    Value *Ptr = getPointerOperand(VL[i]);
----------------
This can probably be replaced with a for range loop:

```
for (auto *Val : VL) {
```

and then replace the uses of VL[i] with Val


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1032
+    const SCEV *OffsetSCEV = SE.getConstant(Offset);
+    const SCEVConstant *OffsetC = dyn_cast<SCEVConstant>(OffsetSCEV);
+    const APInt &constOffset = OffsetC->getAPInt();
----------------
use auto* for dyn_cast return.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1038
+  SmallVector<Value *, 8> list;
+  ArrayRef<Value *> newVL;
+  for (std::multimap<int, Value *>::iterator it = offValPair.begin(),
----------------
newVL is unused?


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1040
+  for (std::multimap<int, Value *>::iterator it = offValPair.begin(),
+                                             e = offValPair.end();
+       it != e; it++) {
----------------
Use auto? You can drop the braces as well.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:466
+      assert(VL.size() == Scalars.size() && "Invalid size");
+      for (unsigned i = 0; i < VL.size(); i++) {
+        bool Found = false;
----------------
This might be simplified with a for range loop and use of llvm:none_of / any_of?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1228
+          }
+        }
+        if (shuffledLoads) {
----------------
Is it worth breaking here once we know that shuffledLoad is false?

Remove the braces if you can.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2582
+            TreeEntry *E = &VectorizableTree[Idx];
+            for (unsigned Lane = 0, LE = VL.size(); Lane != LE; ++Lane) {
+              if (E->Scalars[Lane] == VL[i]) {
----------------
for range loop?


================
Comment at: test/Transforms/SLPVectorizer/X86/jumbled-load.ll:3
+
+
+; CHECK-LABEL: @jumbled-load
----------------
Possibly commit this test to trunk with the current output generated by utils/update_test_checks.py?


================
Comment at: test/Transforms/SLPVectorizer/X86/reduction_loads.ll:8
+; CHECK: mul <8 x i32> <i32 42, i32 42, i32 42, i32 42, i32 42, i32 42, i32 42, i32 42>, [[SHUF]]
 
 define i32 @test(i32* nocapture readonly %p) {
----------------
It'd be better if there was more context to this additional shuffle - regenerate + commit the current output with utils/update_test_checks.py ? For an IR loop it's not that large an output.


https://reviews.llvm.org/D26905