[PATCH] D128897: [SimplifyCFG] Improve SwitchToLookupTable optimization

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

I think I found another miscompile, see the comment about reuseTableCompare. Can we be sure that there are no more? :)

This is why I'm worried about this approach of tweaking how we use the lookup table, and think that perhaps updating the actual table would be a safer approach.



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6373
 
+  uint64_t ZeroOffsetTableSize = MaxCaseVal->getLimitedValue() + 1;
+
----------------
Could this overflow?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6378
   Value *TableIndex;
-  if (MinCaseVal->isNullValue())
+  ConstantInt *Offset;
+  if (MinCaseVal->isNullValue() ||
----------------
Offset is a pretty generic name for a variable that's used in such wide scope. Is there a more descriptive name?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6380
+  if (MinCaseVal->isNullValue() ||
+      (!MinCaseVal->isNegative() && (HasDefaultResults || NeedMask) &&
+       ShouldBuildLookupTable(SI, ZeroOffsetTableSize, TTI, DL, ResultTypes) &&
----------------
This now becomes a pretty complicated condition. I wonder if it could be simplified, or at least made easier to follow somehow. In any case it probably needs a comment about what's going on.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6492
     // possible.
     if (!TableHasHoles && HasDefaultResults && RangeCheckBranch) {
       BasicBlock *PhiBlock = PHI->getParent();
----------------
I think your optimization may break this. Consider:

```
void g();

unsigned char f(int x) {
    unsigned char r;
    switch (x) {
        case 1:  r = 1;  break;
        case 2:  r = 5;  break;
        case 3:  r = 7;  break;
        case 4:  r = 18; break;
        default: r = 0;
    }
    if (r != 0) {
        g();
    }
    return r;
}
```

Because `TableHasHoles` is false, this code and the code in `reuseTableCompare` will conclude that a lookup in the table yields a value that's different from the default, and so the "r != 0" comparison can be replaced with the range check. However, with your optimization we've essentially added "case 0" to the table with the default value.

I believe the result is (though I didn't test it) that with the transformation in this patch, `f(0)` would end up incorrectly calling `g()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128897