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

[Florian Hahn via Phabricator via llvm-commits]

fhahn 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)) {
----------------
jaykang10 wrote:
> 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... 
I think the map change would also be function local. We only need to map the scalar predecessors Here of a phi to the vector predecessors, as suggested in the comment. There should be no need to maintain it before/after this function


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

https://reviews.llvm.org/D80086