[PATCH] D19266: [SCEV] Improve the run-time checking of the NoWrap predicate

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:2020
@@ +2019,3 @@
+  // The expression {Start,+,Step} has nusw/nssw if
+  //   Step < 0, Start - |Step| * Backedge <= Start
+  //   Steo > 0, Start + |Step| * Backedge >= Start
----------------
One of these has to include `Step == 0`.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:2021
@@ +2020,3 @@
+  //   Step < 0, Start - |Step| * Backedge <= Start
+  //   Steo > 0, Start + |Step| * Backedge >= Start
+  // and |Step| * Backedge doesn't overflow.
----------------
Spelling: "Step > 0"

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:2022
@@ -2019,1 +2021,3 @@
+  //   Steo > 0, Start + |Step| * Backedge >= Start
+  // and |Step| * Backedge doesn't overflow.
 
----------------
Nit: I'd be explicit here about "... doesn't **unsigned** overflow"

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:2038
@@ +2037,3 @@
+  Value *StepCompare =
+      Builder.CreateICmp(ICmpInst::ICMP_SLT, StepValue, NegStepValue);
+  Value *AbsStep = Builder.CreateSelect(StepCompare, NegStepValue, StepValue);
----------------
Why not just check if `Step < 0`?  (Assuming later passes don't optimize this) That way we won't have compute `-Step` if `Step` is positive.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:2054
@@ +2053,3 @@
+  //   Start - |Step| * Backedge > Start
+  Value *Add = Builder.CreateAdd(MulV, StartValue);
+  Value *Sub = Builder.CreateSub(StartValue, MulV);
----------------
I'd keep the operands in the same order in both the cases.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:2084
@@ +2083,3 @@
+
+  EndCheck = Builder.CreateAnd(
+      EndCheck, Builder.CreateICmp(ICmpInst::ICMP_NE, StepValue, Zero));
----------------
Why do you need this extra check?  If `Step` is `0` then won't both the less than and the greater than check return false?


http://reviews.llvm.org/D19266