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

[Ayonam Ray via Phabricator via llvm-commits]

ayonam added a comment.

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

> In D52002#1334371 <https://reviews.llvm.org/D52002#1334371>, @ayonam wrote:
>
> > In D52002#1334222 <https://reviews.llvm.org/D52002#1334222>, @hans wrote:
> >
> > > 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?
>
>
> Does the error show with the regular lit tests, or do you have some internal test that fails?
>
> My first guess would be that one of the "unreachable" defaults aren't actually unreachable for some input. But then they should trap in an asserts-enabled build..


No, this shows up in an internal test.  I figured out that the code actually has calls to llvm_unreachable() with an error message, which in a non-debug build, calls __builtin_unreachable().  In a debug build, it would have printed a message.  I'm not sure how to deal with this.  Do you think it is safe to assume that such a behaviour is expected and the test must fail because the inputs are not properly handled or do you think we should handle the unreachable defaults with caution (read conservatively)?  I read up some of the articles on the net on the __builtin_unreachable() behaviour.  They mention that the program should exit, albeit with an error.  However, with our way of handling the unreachable defaults, the program crashes with a segmentation fault.

My take is that we should have a way to mark such unreachable defaults that are caused by a call to __builtin_unreachable() and not omit the branch in those cases and allow the system dependent implementation of __unreachable_default() to handle the manner in which, the program must exit.


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

https://reviews.llvm.org/D52002