[PATCH] D30477: [SCEV] Compute affine range in another way to avoid bitwidth extending.

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

It was not clear to me that this is safe around certain kinds of overflow -- if the questions I raised inline are invalid, please add comments / asserts making them obviously invalid. :)



================
Comment at: lib/Analysis/ScalarEvolution.cpp:4683
+// C <  A <= B
+static bool tripleUCompare(APInt A, APInt B, APInt C) {
+  if (A.ule(B) && B.ult(C))
----------------
`tripleUCompare` is not too descriptive -- what is it actually //doing//?  Rather, what property of `A`, `B`, `C` is it testing for?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4716
+  // By how much the expression can change.
+  APInt Offset = Step * MaxBECount;
+
----------------
Is it okay for this to overflow?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4762
+static ConstantRange getSignedRangeForAR(APInt Step, ConstantRange StartRange,
+                                         APInt MaxBECount, bool BackDirection,
+                                         unsigned BitWidth) {
----------------
Do you need to pass in `BackDirection` here?  Can't you get the same information by checking the sign of `Step`?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4765
+  // By how much the expression can change.
+  APInt Offset = Step * MaxBECount;
+
----------------
Same question here about the multiplication overflowing.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4842
+  ConstantRange SR(BitWidth, /* isFullSet = */ false);
+  if (CheckBackDirection)
+    SR = SR.unionWith(
----------------
I'd s/`CheckBackDirection`/`CheckBwdDirection`/ for consistency.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4844
+    SR = SR.unionWith(
+        getSignedRangeForAR(StepSMin.abs(), StartSRange, MaxBECountValue,
+                            /* BackDirection = */ true, BitWidth));
----------------
Is this correct if `StepSMin` (and thus also `StepSMin.abs()`) is `INT_SMIN`?


https://reviews.llvm.org/D30477