- User Since
- Apr 18 2013, 6:48 AM (300 w, 6 d)
Seems like a good change to me.
Fri, Jan 18
Thu, Jan 17
This (I think) caused a bunch of libc++ tests to start failing during testing of the newly created 8.0 branch on my machine, see https://bugs.llvm.org/show_bug.cgi?id=40354
Wed, Jan 16
I actually tried the patch and got
Here's an example from the buildbots: http://lab.llvm.org:8011/builders/lld-x86_64-win7/builds/31389/steps/build%20lld/logs/stdio
I just tried to build a package, and it failed like this:
I've committed in r351324. That's just after the branch point, but if no problems show up, I'll merge it to the release branch.
Looks good to me. Do you have commit access, or would you like me to commit it for you?
Tue, Jan 15
Sorry for the late reply; I was on vacation. It seems Erich Keane already fixed this in r350941.
Mon, Jan 14
Sorry for the slow response time. I'm back from vacation now.
Sorry for the long delay. I'm back from vacation now, and I'll commit this tomorrow morning unless anyone else objects.
Dec 21 2018
Looks good to me. Do you have commit rights, or do you need someone to commit it for you?
Thanks! Almost there.
Maybe add a release note for out-of-tree users of this? We had to adjust our clang plugin build files in Chromium for example.
Dec 20 2018
The code itself looks good.
Looks promising. Please make the suggested changes, and please move this patch into D52002. The functionality change and the test should be in the same patch.
Dec 19 2018
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.