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

[Kohei Asano via Phabricator via llvm-commits]

khei4 added inline comments.


================
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:
> 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
> 
>  It seems like I missed Max/MinIndex in a signed sense.

I mean we don't need to worry about signed-over/underflow checks with falsy unsigned sense Max/MinIndex.

And I guess [[ https://github.com/llvm/llvm-project/blob/main/llvm/test/Transforms/SimplifyCFG/X86/switch_to_lookup_table.ll#L1695-L1712 | this ]] is index full range tests.
(from above URL)

```
define i32 @signed_overflow1(i8 %n) {
; CHECK-LABEL: @signed_overflow1(
; CHECK-NEXT:  start:
; CHECK-NEXT:    [[TRUNC:%.*]] = trunc i8 [[N:%.*]] to i2
; CHECK-NEXT:    [[SWITCH_TABLEIDX:%.*]] = sub i2 [[TRUNC]], -2
; CHECK-NEXT:    [[SWITCH_TABLEIDX_ZEXT:%.*]] = zext i2 [[SWITCH_TABLEIDX]] to i3
; CHECK-NEXT:    [[SWITCH_GEP:%.*]] = getelementptr inbounds [4 x i32], ptr @switch.table.signed_overflow1, i32 0, i3 [[SWITCH_TABLEIDX_ZEXT]]
; CHECK-NEXT:    [[SWITCH_LOAD:%.*]] = load i32, ptr [[SWITCH_GEP]], align 4
; CHECK-NEXT:    ret i32 [[SWITCH_LOAD]]
;
start:
  %trunc = trunc i8 %n to i2
  switch i2 %trunc, label %bb1 [
  i2 0, label %bb6
  i2 1, label %bb3
  i2 -2, label %bb4
  i2 -1, label %bb5
  ]

bb1:                                              ; preds = %start
  unreachable

bb3:                                              ; preds = %start
  br label %bb6

bb4:                                              ; preds = %start
  br label %bb6

bb5:                                              ; preds = %start
  br label %bb6

bb6:                                              ; preds = %start, %bb3, %bb4, %bb5
  %.sroa.0.0 = phi i32 [ 4444, %bb5 ], [ 3333, %bb4 ], [ 2222, %bb3 ], [ 1111, %start ]
  ret i32 %.sroa.0.0
}

```


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

https://reviews.llvm.org/D150943