In D55616#1335587, @vitalybuka wrote:

Looks like it's broken by this patch

clang: /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm/tools/clang/lib/AST/ExprConstant.cpp:11055: llvm::APSInt clang::Expr::EvaluateKnownConstInt(const clang::ASTContext &, SmallVectorImpl<clang::PartialDiagnosticAt> *) const: Assertion `Result && "Could not evaluate expression"' failed.

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-msan/builds/9281/steps/check-clang%20msan/logs/stdio

@aprantl Is the advantage of your suggested approach just that we don't have to define a new expression type? Obviously the interpretation is not the same as DW_OP_breg on other targets so as you say, either way there would have to be special logic in all the tools that consume it. Is this kind of repurposing of builtin primitives common?

These seems reasonable, although it does also seem like there could be quite a few unintended consequences that we haven't discovered yet.

LGTM.

This seems reasonable to me, although I have a question inline about why you are using makeZeroElementRegion().

LGTM

In D52002#1334543, @hans wrote:

In D52002#1334371, @ayonam wrote:

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

I've verified that this patch fixes the original problem.

In D52002#1335045, @efriedma wrote:

Phabricator has an "upload file" function... or you can just send an email with an attachment to llvm-commits.

Thank you! Splitted into two patches.

Since a similar solution was adopted for clang, I think we should let this one land. After all, it's a matter of consistency between the two projects.

Thanks! Actually landing this is blocked on https://reviews.llvm.org/D55836 though.

I don't use clang. The log of running tests including poisoned_hash_helper.hpp is attached.