[PATCH] D104741: [SCEV] Support single-cond range check idiom in applyLoopGuards.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:13660
 
+    // Check if (LHS Predicate RHS) represent a compare for a range check with a
+    // single condition, as InstCombine may create. A check of the form
----------------
reames wrote:
> I found the wording on this confusing the first several times I read it.  Can you reword?  One suggestion below if helpful.
> 
> Check for a condition of the form (X - 1) < C.  InstCombine will create this form when combining two checks of the form x <= C and x >u 0.  
> 
> (Note the use of the <= to avoid need for C+1 in example.  Note avoid mention "range check" as X > 0 doesn't seem like an idiomatic range check to me.)
Thanks for the suggestion. I updated the wording, but adjusted it slightly to make the lower bound also an arbitrary constant.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:13666
+    //   * X u> 0.
+    if (auto *AddExpr = dyn_cast<SCEVAddExpr>(LHS)) {
+      auto *C1 = dyn_cast<SCEVConstant>(AddExpr->getOperand(0));
----------------
reames wrote:
> Style wise: I'd suggest writing a lambda with the signature
> matchSomeName = [&](SCEV *& X, SCEVConstant *&C).
> 
> I at least would find early return much easier to follow here.
Thanks! I moved the checks + update to the map to a single lambda. Happy to only move the 'match' part as well, if you think that's beneficial.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:13671
+          C1->getValue()->isMinusOne() && Predicate == CmpInst::ICMP_ULT &&
+          C2 && !C2->getAPInt().isMaxValue() && !C2->getAPInt().isNullValue()) {
+        auto *LHSUnknown = dyn_cast<SCEVUnknown>(AddExpr->getOperand(1));
----------------
efriedma wrote:
> Instead of trying to special-case `(-1 + X) u< C`, could you use ConstantRange::makeExactICmpRegion?  Not that this necessarily needs to be generalized, but it would be easier to follow the logic, I think.
Thanks for the suggestion! I updated the code to use `makeExactICmpRegion`, which should be a bit simpler and it should now support arbitrary lower bounds. I think we need to make sure the computed region does not wrap and lower <= upper.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104741