[PATCH] D36113: [Loop Vectorize] Vectorize Loops with Backward Dependence

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: include/llvm/Transforms/IPO/PassManagerBuilder.h:151
   bool LoopVectorize;
+  bool LoopVectorizePred;
   bool RerollLoops;
----------------
DIVYA wrote:
> hfinkel wrote:
> > I don't think we need a special pass-manager option for this. We can put it behind a cl::opt flag, but once it is judged to be ready for the default pipeline, I imagine we'll always enable it whenever vectorization is enabled (at least at -O3).
> I had already added cl::opt flag "EnableLoopVectorizePred" in PassManagerBuilder.cpp to have an option to disable the pass.Do I need add to anything else?
Remove this variable (make the cl::opt default to off for now). We don't want a proliferation of these for each pass.


================
Comment at: include/llvm/Transforms/Vectorize/LoopVectorizePred.h:16
+// to reorder the instructions.
+// For example, the pass reorders the code below, inorder to convert it into
+// vectorizable form.
----------------
inorder -> in order


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:129
+/// Moves load instructions and all its dependences above the corresponding
+/// aliasing store instruction
+bool LoopInstructionsReordering::checkDepAndReorder(StoreInst *StInst,
----------------
Do you mean "aliasing store instruction" or do you mean "backward-dependent store instruction"? I think the latter.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:131
+bool LoopInstructionsReordering::checkDepAndReorder(StoreInst *StInst,
+                                                    Instruction *LdInst) {
+  std::queue<Instruction *> InstQueue;
----------------
Is what you're doing here sufficient? What happens if you have memory-access instructions , function calls, etc. that need to be moved because of (potential) memory dependencies?


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:187
+      if (MSSA->isLiveOnEntryDef(D))
+        Reordered |= checkDepAndReorder(St, Ld);
+      else if (Instruction *DefInst = dyn_cast<Instruction>(D)) {
----------------
I'm somewhat concerned that a number of these cases are actually handling loop-invariant values, meaning that we're doing a suboptimal handling of something that LICM should handle more fully. The problem is that dealing with these after vectorization could be more difficult than before, and if we have a phase-ordering problem such that we're missing these cases, we might just end up with suboptimal code. Are you seeing these cases in practice?



================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:200
+          if (DefPhi) {
+            if (OI.dominates(St, Ld))
+              Reordered |= checkDepAndReorder(St, Ld);
----------------
Are you assuming here that the Phi must be the loop backedge? What if there's control flow in the loop (note that the vectorizer can do if conversion).


https://reviews.llvm.org/D36113