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

[Hideki Saito via Phabricator via llvm-commits]

hsaito added a comment.

Renato/Florian, thanks for the review.

Ideally, we shouldn't be thinking about where to generate bcast code while we are generating vectorized code. We should have determined such a thing before generating a single line of vector code. In a long run, such a decision would be stored somewhere inside the "vectorization plan" and the code gen should just refer to that.



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1743
+  bool NewInstr = Instr &&
+                  !DT->dominates(Instr->getParent(), LoopVectorPreHeader);
   bool Invariant = OrigLoop->isLoopInvariant(V) && !NewInstr;
----------------
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.


================
Comment at: test/Transforms/LoopVectorize/pr37248.ll:1
+; RUN: opt -passes='loop-vectorize' -force-vector-width=2 -S < %s
+;
----------------
fhahn wrote:
> Not sure if the property change above means that test/Transforms/LoopVectorize/pr37248.ll is marked as executable. If that's the case, it would be great if you could remove the property before committing.
> 
> Also, is it worth adding a simple check making sure the loop is vectorized?
Thanks. My bad. copy-paste error. This isn't runnable.

OK. Will check for a vector instruction.


Repository:
  rL LLVM

https://reviews.llvm.org/D46302