[PATCH] D79507: [flang] Change DIE("unreachable") cases to use llvm_unreachable

Wed Jun 3 10:25:37 PDT 2020

mehdi_amini added a comment.

Since flang will not generate code without LLVM, and since LLVM is using assert/unreachable everywhere, the *only* possibility of having a build of flang that is "hardened" with the property that @sscalpone/@tskeith/@klausler is to build LLVM with assertions enabled.
Note also that all the uses of MLIR in Flang will also come with plenty of assert/unreachable calls that won't be able to be turned into "DIE" unless you build with assertions.

So I don't understand why the frontend here would be more specific than the rest of the compiler? The divergence is not solving the problem as I understand it.

> Release builds of the compiler must have well-defined behavior, and that behavior must be to crash on a fatal internal error rather than to silently proceed to generate incorrect code, which is the worst thing a compiler can do to a user. Trading off protection against silent generation of bad code to gain some unmeasured performance improvement would be insanely irresponsible.

Shipping with LLVM_ENABLE_ASSERTIONS=ON wouldn't be enough probably, you likely want to ship with UBSAN enabled. The good news is that no one prevents you from doing so!

In D79507#2056597 <https://reviews.llvm.org/D79507#2056597>, @sscalpone wrote:

> In D79507#2055540 <https://reviews.llvm.org/D79507#2055540>, @DavidTruby wrote:
>
> > This was originally the approach taken by Kiran from AMD in this patch: https://github.com/flang-compiler/f18/pull/1057
> >  It was rejected pending the aforementioned discussion about doing things the way LLVM does them currently vs changing the LLVM policy.
>
>
> That doesn't seem to be the case.  Quoting:
>
> > ...that there is no benefit of partially replacing die function with llvm_unreachable. I will go ahead and close this PR.

The most recent discussions was likely: https://github.com/flang-compiler/f18/issues/966

I'll quote that I was OK with the merge of f18 into LLVM under the condition that:

> I think it is not a blocker as long as there is agreement that merging right now implies the LLVM ways of modeling assertions, and having a "release_assert" is something that you'll need to convince to change on llvm-dev@ later.

So right now the default should be the LLVM policy.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79507