[PATCH] D127482: [SimplifyCFG] Share code to compute switch density between ShouldBuildLookupTable() and ReduceSwitchRange()

[Hans Wennborg via Phabricator via llvm-commits]

hans added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6119
                        const SmallDenseMap<PHINode *, Type *> &ResultTypes) {
-  if (SI->getNumCases() > TableSize || TableSize >= UINT64_MAX / 10)
-    return false; // TableSize overflowed, or mul below might overflow.
----------------
thakis wrote:
> hans wrote:
> > thakis wrote:
> > > Before we could handle table size up to `UINT64_MAX / 10`, after only up to `UINT64_MAX / 100`. That's not "no functionality change", is it?
> > I think it's still NFC in practice; we'd return false for table sizes of that magnitude anyway.
> Should we just remove all the overflow checks then?
No, you can still trigger overflows here, and overflowing would cause us to return the wrong answer.

For example:

```
switch (x) {
case 0: foo();
case VERY_BIG_NUMBER: bar();
}
```

TableSize will be VERY_BIG_NUMBER + 1, so overflowing when multiplying with 10 or 100 is a real concern.

But in practice, for huge table sizes, we'd return false later anyway, since it's not practically possible to have enough cases that the switch would be considered dense enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127482