[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)) {
----------------
jaykang10 wrote:
> fhahn wrote:
> > 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
> I am sorry for late. Let's see the basic blocks of the phi from attached example.
> 
> OrigPhi's BasicBlock dump
> 
> ```
> for.body10.preheader:        ; preds = %for.body4.lr.ph, %for.cond.cleanup9
>   %indvars.iv60 = phi i64 [ 0, %for.body4.lr.ph ], [ %indvars.iv.next61, %for.cond.cleanup9 ]
> ...
> In this basic block's loop
> %for.body4.lr.ph is preheader
> %for.cond.cleanup9 is latch
> ```
> 
> NewPhi's BasicBlock dump
> 
> ```
> for.body10.preheader1:       ; preds = %for.cond.cleanup96, %vector.body**
>   %vec.phi = phi <4 x i64>
> ...
> In this basic block's loop
> %for.cond.cleanup96 is latch
> %vector.body is preheader
> ```
> As you can see, the order of predecessor is different between the scalar and vector basic blocks. The scalar one's predecessor order is preheader first and latch next but the vector one is opposite. I am not sure whether it works to just map the scalar basic block to vector basic block in this function or not. I guess we could map it where we create the vector phi or somewhere we have the correct information. If I missed something, please let me know.
Additiionally, for the vector basic block, the first predecessor is generated by creating preheader's terminator. The second one is generated by setting up latch's successor. 


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

https://reviews.llvm.org/D80086