[PATCH] D89489: [InterleaveAccess] Recognize Interleave loads through binary operations

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

I haven't looked at this pass much, so just pointed out some minors. 
There are tests specifically for this pass in `test/Transforms/InterleavedAccess/AArch64`. So it would be nice to have minimal IR tests using "opt < %s -interleaved-access". If possible, create a test where a load has both shuffle users and shuffle-of-binop users, so we test a scenario where both of those data structures are populated.



================
Comment at: llvm/lib/CodeGen/InterleavedAccessPass.cpp:295-297
   SmallVector<ShuffleVectorInst *, 4> Shuffles;
   SmallVector<ExtractElementInst *, 4> Extracts;
+  SmallSetVector<ShuffleVectorInst *, 4> BinOpShuffles;
----------------
Should all of these be SmallSetVector?


================
Comment at: llvm/lib/CodeGen/InterleavedAccessPass.cpp:301
+  // users that are extractelement instructions or binary operators, we save
+  // them to later check if they can be modifed to extract from one of the
+  // shufflevectors instead of the load.
----------------
typo: modified


================
Comment at: llvm/lib/CodeGen/InterleavedAccessPass.cpp:311
+    if (BI && BI->hasOneUse()) {
+      if (auto SVI = dyn_cast<ShuffleVectorInst>(*BI->user_begin())) {
+        BinOpShuffles.insert(SVI);
----------------
auto -> auto *


================
Comment at: llvm/lib/CodeGen/InterleavedAccessPass.cpp:316
+    }
+    ShuffleVectorInst *SVI = dyn_cast<ShuffleVectorInst>(User);
     if (!SVI || !isa<UndefValue>(SVI->getOperand(1)))
----------------
Use `auto *` here too for consistency.


================
Comment at: llvm/lib/CodeGen/InterleavedAccessPass.cpp:343
   // and factor as the first shufflevector.
-  for (unsigned i = 1; i < Shuffles.size(); i++) {
+  for (unsigned i = 0; i < Shuffles.size(); i++) {
     if (Shuffles[i]->getType() != VecTy)
----------------
for (unsigned i =0, e = Shuffle.size(); i != e; ++i) {


================
Comment at: llvm/lib/CodeGen/InterleavedAccessPass.cpp:352
   }
+  for (unsigned i = 0; i < BinOpShuffles.size(); i++) {
+    if (BinOpShuffles[i]->getType() != VecTy)
----------------
Similar to above - http://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop


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

https://reviews.llvm.org/D89489