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

[Michael Zolotukhin via Phabricator via llvm-commits]

mzolotukhin marked 3 inline comments as done.
mzolotukhin added a comment.

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

Absolutely.

I updated the patch, please take a look when you are ready :)

Thanks,
Michael



================
Comment at: lib/Analysis/ScalarEvolution.cpp:4691
+    return StartRange;
+  if (StartRange.isFullSet())
+    return StartRange;
----------------
sanjoy wrote:
> 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 ..`
I prefer to keep them separate for the following reason. In the first case we check if the start range doesn't change at all. I.e. we know for sure that final value is the same as the original one because either step is 0, or MaxBECount is 0. In the second case, in contrast, the value is changed, but we cannot predict a new range, because the original range is too conservative. I.e. the final range is also full range, in some sense it's a different full range compared to the start range. I hope this explanation makes sense :)


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4696
+  // note that we're moving in the opposite direction.
+  bool BackDirection = Signed && Step.isNegative();
+  if (Signed)
----------------
sanjoy wrote:
> 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).
> 
> I'd s/BackDirection/Descending/
I like `Descending` better too, thanks!

> Also, please add a justification on why this abs does the right thing of Step is INT_SMIN
I tried to add a comment about it, but I'm not sure it's clear enough. But I'm pretty sure we're doing correct thing here due to wrap-around of APInt.


================
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);
----------------
sanjoy wrote:
> Can we get the `APInt::getMaxValue(StartRange.getBitWidth()) == MaxBECount * Step` check here by changing `ult` to `ule`?
I rechecked the formulas and I think the second one is not need at all. 

If MaxBECount*Step == UINT_MAX, then we don't overflow. E.g. we can go from 0 to 255 with step=1 and MaxBECount=255 without a wrap.


================
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;
----------------
sanjoy wrote:
> I'd suggest not calling these `*Min` or `*Max` since they're not the minimum or maximum (signed or unsigned).
Thanks, fixed.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4717
+  APInt StartMax = StartRange.getUpper() - 1;
+  APInt MovedBoundary = BackDirection ? StartMin - Offset : StartMax + Offset;
+
----------------
sanjoy wrote:
> 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)`
Fixed.


https://reviews.llvm.org/D30477