[PATCH] D13087: A fix for loop vectorizer with handling loops with volatile induction variables

[hfinkel@anl.gov via llvm-commits]

hfinkel added inline comments.

================
Comment at: include/llvm/Transforms/Utils/Local.h:139
@@ -137,3 +138,3 @@
 /// the basic block that was pointed to.
 ///
 bool SimplifyCFG(BasicBlock *BB, const TargetTransformInfo &TTI,
----------------
Please update the comment to explain the new parameter. As is, it is not even clear whether this is an optional input or output.

================
Comment at: include/llvm/Transforms/Utils/Local.h:141
@@ -139,3 +140,3 @@
 bool SimplifyCFG(BasicBlock *BB, const TargetTransformInfo &TTI,
-                 unsigned BonusInstThreshold, AssumptionCache *AC = nullptr);
+                 unsigned BonusInstThreshold, AssumptionCache *AC = nullptr, SmallPtrSet<BasicBlock*, 16> * LoopHeaders = nullptr);
 
----------------
This line is too long.

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:208
@@ -208,1 +207,3 @@
+          BB->getFirstNonPHIOrDbg()->isTerminator()  &&
+	  !LoopHeaders.count(BB)) {		// if the BB is a loop header, don't eliminate 
         // Since TryToSimplifyUncondBranchFromEmptyBlock may delete the
----------------
Indentation is odd here (no tabs). Also, we should explain why we should not eliminate the loop header. Saying something like, "because eliminating a loop header might later prevent LoopSimplify from transforming nested loops into simplified form" is probably sufficient.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:137
@@ -135,3 +136,3 @@
   SimplifyCFGOpt(const TargetTransformInfo &TTI, const DataLayout &DL,
-                 unsigned BonusInstThreshold, AssumptionCache *AC)
-      : TTI(TTI), DL(DL), BonusInstThreshold(BonusInstThreshold), AC(AC) {}
+                 unsigned BonusInstThreshold, AssumptionCache *AC, SmallPtrSet<BasicBlock*, 16> * LoopHeaders)
+      : TTI(TTI), DL(DL), BonusInstThreshold(BonusInstThreshold), AC(AC), LoopHeaders(LoopHeaders) {}
----------------
Line too long.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:4622
@@ -4616,2 +4621,3 @@
   if (I->isTerminator() && BB != &BB->getParent()->getEntryBlock() &&
+      LoopHeaders && !LoopHeaders->count(BB) &&
       TryToSimplifyUncondBranchFromEmptyBlock(BB))
----------------
You're changing the behavior here when LoopHeaders is not passed in. Is that what you intend? If you don't intend that, then I assume you want:

  (!LoopHeaders || !LoopHeaders->count(BB))

if you do intend this, then this must be carefully documented and explained in the interface comments.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:4884
@@ -4877,3 +4883,3 @@
 bool llvm::SimplifyCFG(BasicBlock *BB, const TargetTransformInfo &TTI,
-                       unsigned BonusInstThreshold, AssumptionCache *AC) {
+                       unsigned BonusInstThreshold, AssumptionCache *AC, SmallPtrSet<BasicBlock*, 16> * LoopHeaders) {
   return SimplifyCFGOpt(TTI, BB->getModule()->getDataLayout(),
----------------
Line is too long.


Repository:
  rL LLVM

http://reviews.llvm.org/D13087