[PATCH] D146903: [SimplifyCFG] add nsw on SwitchToLookupTable Index calculation

[Kohei Asano via Phabricator via llvm-commits]

khei4 added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6512
     TableIndexOffset = MinCaseVal;
-    TableIndex =
-        Builder.CreateSub(SI->getCondition(), TableIndexOffset, "switch.tableidx");
+    TableIndex = Builder.CreateSub(SI->getCondition(), TableIndexOffset,
+                                   "switch.tableidx", /*HasNUW =*/false,
----------------
nikic wrote:
> Add something like `// If the default is unreachable, all case values are s>= MinCaseVal, so the subtraction cannot wrap in a signed sense.`
> 
> This part of the change looks correct to me. It might be worthwhile to split it from the changes to BuildLookup.
> Add something like // If the default is unreachable, all case values are s>= MinCaseVal, so the subtraction cannot wrap in a signed sense.


Thanks! I'll basically took this comment as it is


================
Comment at: llvm/test/Transforms/SimplifyCFG/X86/switch-table-bug.ll:12
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[SWITCH_TABLEIDX:%.*]] = sub i2 [[TMP0:%.*]], -2
+; CHECK-NEXT:    [[SWITCH_TABLEIDX:%.*]] = sub nsw i2 [[TMP0:%.*]], -2
 ; CHECK-NEXT:    [[SWITCH_TABLEIDX_ZEXT:%.*]] = zext i2 [[SWITCH_TABLEIDX]] to i3
----------------
nikic wrote:
> nikic wrote:
> > This is a miscompile: E.g. if the input was 0 then we'll do 0 - (-2) which should be 2 but overflows to -2.
> > 
> > The bit I missed here is that we're effectively mapping a signed range to an unsigned one, doing something like -2 -> 0, -1 -> 1, 0 -> 2, 1 -> 3, but of course the last two become -2 and -1 when interpreted as signed.
> > 
> > I think we need an additional check that TableSize is small enough (should not be larger than half the integer space, or something like that).
> Probably the simplest way to check this is to do something like `MaxCaseVal->getValue().ssub_ov(MinCaseVal->getValue())` and check that it doesn't overflow. That directly corresponds to the operation we're doing...
It seems right! Thank you for a good catch!
> Probably the simplest way to check this is to do something like MaxCaseVal->getValue().ssub_ov(MinCaseVal->getValue()) and check that it doesn't overflow.
Yeah, it seems like repetitions though, seem simpler than 
`1 << (TableIndexOffset->getBitWidth() - 1) < TableSize - 1`. :)



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

https://reviews.llvm.org/D146903