[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.

Mostly minor stuff.  Requesting another iteration mostly because I too want to look at it again with fresh eyes to see if I missed anything.



================
Comment at: lib/Analysis/ScalarEvolution.cpp:4691
+    return StartRange;
+  if (StartRange.isFullSet())
+    return StartRange;
----------------
How about just folding all of these together and changing the comment to match?

`If either Step or MaxBECount is 0 or the start range is maximally conservative to begin with ..`


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4696
+  // note that we're moving in the opposite direction.
+  bool BackDirection = Signed && Step.isNegative();
+  if (Signed)
----------------
I'd s/`BackDirection`/`Descending`/

Also, please add a justification on why this `abs` does the right thing of `Step` is `INT_SMIN` (or bail out in that case).



================
Comment at: lib/Analysis/ScalarEvolution.cpp:4702
+  // expression is guaranteed to overflow.
+  if (APInt::getMaxValue(StartRange.getBitWidth()).udiv(Step).ult(MaxBECount))
+    return ConstantRange(BitWidth, /* isFullSet = */ true);
----------------
Can we get the `APInt::getMaxValue(StartRange.getBitWidth()) == MaxBECount * Step` check here by changing `ult` to `ule`?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4715
+  // if the expression is decreasing and will be increased by Offset otherwise.
+  APInt StartMin = StartRange.getLower();
+  APInt StartMax = StartRange.getUpper() - 1;
----------------
I'd suggest not calling these `*Min` or `*Max` since they're not the minimum or maximum (signed or unsigned).


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4717
+  APInt StartMax = StartRange.getUpper() - 1;
+  APInt MovedBoundary = BackDirection ? StartMin - Offset : StartMax + Offset;
+
----------------
Very minor stylistic thing (and I'll understand if you don't want to change it): I'd have written this as `BackDirection ? (StartMin - Offset) : (StartMax + Offset)`


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4725
+
+  APInt NewMin, NewMax;
+  if (BackDirection) {
----------------
Again, these should not be called `*Min` or `*Max`.


https://reviews.llvm.org/D30477