[PATCH] D60093: [LoopPredication] Allow predication of loop invariant computations

[Artur Pilipenko via Phabricator via llvm-commits]

apilipenko added a comment.

1. Since we need to fiddle with the insertion point I think we should stop using single IRBuilder which we pass around. It looks like creating a local IRBuilder is a cheap operation, so we can have local IRBuilders when we need to generate instructions. This way the insertion point will be clear from the local context.

2. Invariant expression analysis over IR is a bit non-trivial. It looks like extending SCEV would be a simpler option. Alternatively, we can build up the complexity of the IR analysis incrementally as I suggest in the comment below.



================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:408
 
-  if (SE->isLoopEntryGuardedByCond(L, Pred, LHS, RHS))
-    return Builder.getTrue();
-  if (SE->isLoopEntryGuardedByCond(L, ICmpInst::getInversePredicate(Pred),
-                                   LHS, RHS))
-    return Builder.getFalse();
+  if (SE->isLoopInvariant(LHS, L) && SE->isLoopInvariant(RHS, L)) {
+    if (SE->isLoopEntryGuardedByCond(L, Pred, LHS, RHS))
----------------
Nit. Extract `SE->isLoopInvariant(LHS, L) && SE->isLoopInvariant(RHS, L)` into a local variable? You check it below as well.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:418
+  if (SE->isLoopInvariant(LHS, L) && SE->isLoopInvariant(RHS, L))
+    InsertAt = L->getLoopPreheader()->getTerminator();
   Value *LHSV = Expander.expandCodeFor(LHS, Ty, InsertAt);
----------------
You can use `Preheader` cached in the field.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:479
+  // are presumably rare, and hint at missing oppurtunities for other passes.
+  S->dump();
+  dbgs() << isSafeToExpandAt(S, At, *SE) << "\n";
----------------
Debug printing.


================
Comment at: lib/Transforms/Scalar/LoopPredication.cpp:992-1003
+  while (!Worklist.empty()) {
+    Value *Item = Worklist.pop_back_val();
+    InvariantExpressions.insert(Item);;
+    for (User *U : Item->users()) {
+      Instruction *User = cast<Instruction>(U);
+      if (!L->contains(User->getParent()))
+        continue;
----------------
The complexity to handle cases when the expression is a non-side effecting operation with invariant inputs deserves a test.

Alternatively, you can introduce this complexity as a follow up. In the initial patch you can handle only loads, which should cover most of the cases.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60093/new/

https://reviews.llvm.org/D60093