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

[Nikita Popov via Phabricator via llvm-commits]

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

LGTM



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:13664
+      auto *AddExpr = dyn_cast<SCEVAddExpr>(LHS);
+      if (!AddExpr)
+        return false;
----------------
I would check `|| AddExpr->getNumOperands() != 2` already here...


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:13668
+      auto *C1 = dyn_cast<SCEVConstant>(AddExpr->getOperand(0));
+      auto *C2 = dyn_cast<SCEVConstant>(RHS);
+      if (AddExpr->getNumOperands() != 2 || !C1 || !C2)
----------------
... and then fetch LHSUnknown and check it already here. That seems like a more obvious grouping of conditions.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:13688
+          getConstant(ExactRegion.getLower()),
+          getUMinExpr(RewrittenLHS, getConstant(ExactRegion.getUpper())));
+      return true;
----------------
fhahn wrote:
> nikic wrote:
> > Ranges are half-open, so I don't think getUpper() is correct here. I recommend using getUnsignedMin() and getUnsignedMax() instead. If you do that, then the wrapped set check is also no longer relevant for correctness.
> That's better thanks. I still kept the wrapped check because re-writing is unlikely to add additional info for wrapped ranges.
Right. A future extension here could be to check whether it's wrapping but not sign-wrapping, in which case you could do the same with smin/smax.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104741