[PATCH] D29034: [Guards] Introduce loop-predication pass

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

Looks great!

I have some nitpicky comments inline, but feel free to push after addressing them without further review (or address them in follow-up commits).



================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:1
+//===-- LoopPredication.cpp - ---------------------------------------------===//
+//
----------------
Please edit the header to include a description like in other .cpp files.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:59
+namespace {
+class LoopPredication : public LoopPass {
+public:
----------------
Now is a good time to make sure this pass also runs with the new pass manager.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:129
+    return None;
+ if (!IndexAR->isAffine())
+   return None;
----------------
Why do you care about `!IndexAR->isAffine()` here?


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:143
+  // false as the loop progresses, so take the value on the last iteration.
+  const SCEV *NewIndexSCEV =
+      IsIncreasing ? IndexAR->getStart()
----------------
I'd s/`NewIndexSCEV`/`NewLHS`/ since it isn't an index any more.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:146
+                   : SE->getSCEVAtScope(IndexAR, L->getParentLoop());
+
+  if (!SE->isLoopInvariant(NewIndexSCEV, L) ||
----------------
Since you invoked `getSCEVAtScope` (which can fail), you should check (and bail) if `NewIndexSCEV` was `SCEVCouldNotCompute`.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:159
+
+bool LoopPredication::processGuard(IntrinsicInst *Guard,
+                                   SCEVExpander &Expander) {
----------------
`processGuard` basically says nothing about what this function does.  "Processing" them to what end? :)

I'd call this `widenGuard` or `predicateUsingGuard` or something like that.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:170
+  // widened across loop iterations. Widening these conditions remember the
+  // resuting list of subconditions in Checks vector.
+  SmallVector<Value *, 4> Worklist(1, Guard->getOperand(0));
----------------
s/resuting/resulting/


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:177
+  unsigned NumWidened = 0;
+  while (Worklist.size() != 0) {
+    Value *Condition = Worklist.pop_back_val();
----------------
This might be better as as `do-while` loop, since you know you'll run at least one iteration.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:191
+    if (ICmpInst *ICI = dyn_cast<ICmpInst>(Condition)) {
+      auto NewRangeCheck = widenICmpRangeCheck(ICI, Expander, Builder);
+      if (NewRangeCheck) {
----------------
Use `auto *` to emphasize that you'll get back a pointer.

Actually, I'd rather spell out the type here as `Value *` (since it is not 100% obvious what `widenICmpRangeCheck` returns), and instead use `auto *` for the result of of the `dyn_cast` in the if expression.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:209
+  Value *LastCheck = nullptr;
+  for (auto Check : Checks)
+    if (!LastCheck)
----------------
Use `auto *`.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:248
+  bool Changed = false;
+  for (auto Guard : Guards)
+    Changed |= processGuard(Guard, Expander);
----------------
`auto *`.


https://reviews.llvm.org/D29034