[PATCH] D46302: [LV] Fix for PR37248, Broadcast codegen incorrectly assumed vector loop body is single basic block

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1743
+  bool NewInstr = Instr &&
+                  !DT->dominates(Instr->getParent(), LoopVectorPreHeader);
   bool Invariant = OrigLoop->isLoopInvariant(V) && !NewInstr;
----------------
hsaito wrote:
> fhahn wrote:
> > IIUC we want to check here if Instr is part of the vector loop, right? Wouldn't it be more straight forward to check if the vector loop header dominates Instr?
> Talking about straightforwardness, I suppose the most straightforward way to say it's okay to generate bcast in preheader would be "the value comes from outside of the loop". Instead of computing "Invariant" in a convoluted way, we could do
> 
> bool SafeToHoist = OrigLoop->isLoopInvariant(V) &&
>                                 (!Instr || DT->dominates(Instr->getParent(), LoopVectorPreHeader));
> 
> A lot cleaner this way?
> 
> There is one caveat. DT is still not fully up-to-date within the vectorizer's code gen
> (e.g., PR37221 hits that problem). As far as the LoopVectorPreHeader and the code outside of the vectorized loop are concerned, it is up-to-date (emitMinimumIterationCountCheck() does that). That's why I think "Instr dominates preheader" check can be safely used here.
> 
> At this moment, it's best not to rely on DT w.r.t. instructions inside loop body. If not found (incorrectly), DT will return false and that'll end up in bcast code in preheader, causing a stability issue. So, using "Instruction dominates PH" is more defensive coding.
Right thanks Hideki! IMO the "SafeToHoist" variant is cleaner.


Repository:
  rL LLVM

https://reviews.llvm.org/D46302