[PATCH] D35139: [SLPVectorizer][InstCombine] Fix PR21780 Expansion of 256 bit vector loads fails to fold into shuffles

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

First pass review



================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:93
+
+DenseMap<Value *, std::pair<Value*, Value*>> GEPs;
+
----------------
You really shouldn't have global variables like this, these need to go and be embedded in the combine instead.


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:1423
 
+  if (!dyn_cast<VectorType>(Ops[0]->getType()) && GEP.getNumOperands() == 2)
+    if (Constant *CST = dyn_cast<Constant>(GEP.getOperand(1))) {
----------------
Comments would be nice!


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2861
+        BasicBlock *BB = Load->getParent();
+        for (BasicBlock::iterator Iter = BB->begin(),
+                            E = BB->end(); Iter != E; ) {
----------------
Can you use auto instead of BasicBlock::iterator ?


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2869
+           if (Bases[LInstr->getOperand(0)])
+              MaxOffset = (dyn_cast<ConstantInt>
+                     (Bases[LInstr->getOperand(0)]))->getZExtValue();
----------------
use cast<> not dyn_cast<> if you know the cast will succeed.


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2874
+             MaxOffset = (dyn_cast<ConstantInt>
+                  (Bases[GEPs[LInstr->getOperand(0)].first]))->getZExtValue();
+             CurPos = (dyn_cast<ConstantInt>(
----------------
cast<>


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2876
+             CurPos = (dyn_cast<ConstantInt>(
+                         GEPs[LInstr->getOperand(0)].second))->getZExtValue();
+           }
----------------
cast<> 


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2882
+                    APInt(64, (MaxOffset - CurPos)
+                              *DL.getTypeStoreSize(LInstr->getType())));
+             SmallVector<Metadata*, 3> Vals;
----------------
Do the APInt offset calc separately so this code is clearer.


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2891
+        }
+      }
       eraseInstFromFunction(*I);
----------------
clang-format all this whole new code


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4872
+        Ptr = LInstr->getOperand(0);
+     ConstantAsMetadata *ValMD =
+                 dyn_cast<ConstantAsMetadata>(MD->getOperand(0));
----------------
Newline after the Ptr = line to make it clearer, use cast<>?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4969
+    return nullptr;
+}
+
----------------
clang-format


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:5138
+           findBuildVector(Insert, BuildVector, BuildVectorOpds);
+      }
+
----------------
clang-format (drop the braces?)


================
Comment at: test/Transforms/SLPVectorizer/X86/21780.ll:1
+; RUN: opt < %s -instcombine -slp-vectorizer -mtriple=x86_64-unknown-linux-gnu -mcpu=bdver2 -S | FileCheck %s --check-prefix=SLP
+; RUN: opt < %s -instcombine -mtriple=x86_64-unknown-linux-gnu -mcpu=bdver2 -S | FileCheck %s --check-prefix=ICOMB
----------------
Test file should be called pr21789.ll.

Submit it to trunk now with current codegen and then update this patch so it shows the deltas - you can use utils/update_test_checks.py to auto-generate the codegen


https://reviews.llvm.org/D35139