[PATCH] D52002: Switch optimization for known maximum switch values

[Ayonam Ray via Phabricator via llvm-commits]

ayonam added a comment.

In D52002#1334222 <https://reviews.llvm.org/D52002#1334222>, @hans wrote:

> In D52002#1332378 <https://reviews.llvm.org/D52002#1332378>, @ayonam wrote:
>
> > @hans
> >
> > The earlier code had a problem in that if we converted every switch that had an unreachable default, to omit the check for the default values beyond the maximum switch case value given in the code, then there were some corner cases where the code generated was semantically incorrect and resulted in runtime failures. These failures are happening while compiling LLVM code itself.
>
>
> We need to figure out what those failures were. If the unreachable default was indeed reachable, that's a big problem.
>
> > So I have added a boolean in the SwitchInst class to mark switches that have been widened, so that we omit the branch only for those switches.
>
> LLVM doesn't put this kind of optimization metadata in the IR. The right thing to do is to figure out why removing the branch for unreachable default didn't work, and fix it.
>
> If you can provide more details about what didn't work, maybe I can help investigate. (Though I'm about to go on holiday soon, so probably not until January.)


There are three files that when compiled with this patch, generate wrong code, viz., AArch64LoadStoreOptimizer.cpp, AArch64InstrInfo.cpp and AArch64ConditionalCompares.cpp.  Out of these we tried to isolate the problem with the last one.  I figured out that if the functions SSACCmpConv::findConvertibleCompare() and SSACCmpConv::convert() are compiled without this patch, the code works fine.  So the problem surfaces with these two routines only.  There are a few switch cases in those two routines but I couldn't see anything exceptional with those except for a call to __builtin_unreachable() in the default case for two of the switches and a [[clang::fallthrough]] in another.  In all these three cases, I was unable to figure out how they could possibly break our assumptions.  Does the __builtin_unreachable() have any special semantic that we are not handling?


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

https://reviews.llvm.org/D52002