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

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

>>> 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.

Sounds like that's the bug that needs to be fixed, then.

> 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.

I'm not sure what articles you read, but the documented behaviour for __builtin_unreachable() is that if it's reached, the program has undefined behaviour, the point being that the compiler may assume it's really unreachable.

> 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.

No, unreachable means unreachable and the compiler should treat it as such. There's nothing that says the program needs to exit, either with a segfault or anything else, when hitting __builtin_unreachable().


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

https://reviews.llvm.org/D52002