[PATCH] D80086: [VPlan] Fix wrong vec.phi generation

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 marked an inline comment as done.
jaykang10 added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4068
+      // Check dominance of NewIncV with NewPredBB.
+      if (Instruction *NewIncI = dyn_cast<Instruction>(NewIncV)) {
+        if (!DT->dominates(NewIncI, NewPredBB)) {
----------------
fhahn wrote:
> This seems a bit more complicated than necessary I think
> 
> IIUC, if we have a map ScalarPred -> VectorPred (lets call it ScalarPred2VectorPred) we can get the blocks in the loop as follows:
> 
> ```
> BasicBlock *ScalarBB = OrigPhi->getIncomingBlock(i);
> BasicBlock *VectorBB = ScalarPred2VectorPred[ScalarBB]; // (also assert that ScalarBB is in the map)
> ```
> 
> Building ScalarPred2VectorPred should be straight-forward IIUC, as we should be able to rely on the predecessor order being the same between vector and scalar versions: just set `ScalarPred2VectorPred[ScalarBBPredecessors[I]] = VectorBBPredecessors[I];` You also don't have to ScalarBBPredecessors/VectorBBPredecessors directly. You can just iterate over the predecessors for both and populate the map (you might want to use llvm::zip from STLExtras.h)
As you mentioned, I could create the map to keep the scalar BB and vector BB... I thought the dominate tree simpler solution to fix this bug because we need to change only this function rather than places where we need to update the new map... I would like to get other reviewers' opinion about this... 


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

https://reviews.llvm.org/D80086