[PATCH] D69617: [LoopUnroll] countToEliminateCompares(): fix handling of [in]equality predicates (PR43840)

[Florian Hahn via Phabricator via llvm-commits]

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

LGTM, with a few outstanding comments!

I think some cases could still slip through the cracks, but the most important negative cases are covered by the test cases.



================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollPeel.cpp:216
     // consider AddRecs of the loop we are trying to peel and avoid
     // non-monotonic predicates, as we will not be able to simplify the loop
     // body.
----------------
please update the comment, it still mentions we not supporting non-monotonic predicates.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollPeel.cpp:241
+    const SCEV *NextIterVal = SE.getAddExpr(IterVal, Step);
+    auto PeelOneMoreIteration = [&]() {
+      IterVal = NextIterVal;
----------------
IMO it would be slightly better to make the captures explicit (here and below)


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollPeel.cpp:255
+
+    // With *that* peel count, does the predicate !Pred becomes known in the
+    // first iteration of the loop body after peeling?
----------------
nit: ...does the predicate .... become known...


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollPeel.cpp:259
+                             RightSCEV))
+      continue; // If not, we're done here.
+
----------------
maybe replace  'we're done here' with' Give up'. like below, to make it clear that we do not want to peel for the given condition.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollPeel.cpp:267
+        SE.isKnownPredicate(Pred, NextIterVal, RightSCEV) &&
+        !SE.isKnownPredicate(ICmpInst::getInversePredicate(Pred), NextIterVal,
+                             RightSCEV)) {
----------------
I think we don't need to check the inverse predicate is known, when we already know the regular predicate is known, which is the previous check. Similarly, we already know know `SE.isKnownPredicate(ICmpInst::getInversePredicate(Pred), IterVal, RightSCEV)`, so we should not need to check `!SE.isKnownPredicate(Pred, IterVal, RightSCEV) `

It might be worth having those 2 as an assertion though, in case I am missing something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69617