[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:13677
+      if (ExactRegion.isWrappedSet() || ExactRegion.isFullSet() ||
+          ExactRegion.getLower().ugt(ExactRegion.getUpper()))
+        return false;
----------------
nikic wrote:
> Checking getLower/getUpper here shouldn't be necessary, as you already check for a wrapped set.
good point, removed.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:13688
+          getConstant(ExactRegion.getLower()),
+          getUMinExpr(RewrittenLHS, getConstant(ExactRegion.getUpper())));
+      return true;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104741