[PATCH] D150943: [SimplifyCFG] add nsw on BuildLookuptable LinearMap calculation

[Kohei Asano via Phabricator via llvm-commits]

khei4 added a comment.

I would skip the LinearMap part! Until the overflow benefits are revealed!G



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6122
+    LinearOffset->getValue().sadd_ov(
+        LinearMultiplier->getValue().smul_ov(MaxIndex, MulSW), AddSW);
     if (!LinearMultiplier->isOne())
----------------
khei4 wrote:
> nikic wrote:
> > I don't think this is sufficient. Let's say we have multiplier 2 and MaxIndex is -1 (that is, full range). Then 2*-1 does not overflow, but there are still plenty of values in the range that do overflow (e.g. INT_MAX).
> > 
> > A possible way to properly handle this would be using ConstantRange. I think another possibility is to used ssub_ov for the `Val - PrevVal` calculation above and set NSW based on the overflow flag there.
> > 
> > In any case, we need additional tests to cover this.
> > Let's say we have multiplier 2 and MaxIndex is -1 (that is, full range). Then 2*-1 does not overflow, but there are still plenty of values in the range that do overflow (e.g. INT_MAX).
> 
> Yeah, its case matters. Overflowed MaxIndex should be handled. I'll use ConstantRange! 
> 
> Also, I'll add a linear map test that uses full range.
@nikic It seems like I missed [[ https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/SimplifyCFG.cpp#L6411-L6414 | Max/MinIndex is signed sense ]].
I think the tests we need are unsigned results with signed-overflowed/underflowed values



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

https://reviews.llvm.org/D150943