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

[David Truby via Phabricator via llvm-commits]

DavidTruby added a comment.

I agree with @hfinkel and should have changed these strings to be more descriptive; I just wanted to get this merged as soon as possible so that the general discussion about error handling could happen. I thought we had agreed to go ahead with this change as a prelude to discussing error handling in general.

> Hi David, what *is* the runtime behavior of llvm_unreachable in a program compiled with RELEASE?

The runtime behaviour when compiled with RELEASE is undefined. As I and @hfinkel mentioned, the point of that is optimisations that cannot otherwise be made by the compiler. I would say that runtime performance of release builds should be one of our main priorities...

> are these unreachable because they correspond to things that just haven't been implemented yet?

They're unreachable because they can't actually be arrived at with the current design of traversing the parse tree. I'm not sure why the functions exist in this case though to be honest, since they can't be called. Someone with more knowledge of this code than me can probably answer that question better.

> This is the crux of my concerns. It has been determined, presumably by measurement, that clang's -fsyntax-only benefits enough from this optimization that it is worth the cost of a worse user experience when the internal error occurs (undefined behavior vs. internal error message with file and line number).

There can be no internal error in any of these cases. We're not using `llvm_unreachable` to state that these //shouldn't// be unreachable. They actually //are not// reachable through any code path, other than the `Pre` functions which you will discover you have called incorrectly //very quickly// in Debug builds. It's also not simply a premature optimisation; there's a huge amount of prior art for unreachable optimisations being very effective.

There's also the secondary point that we should align with the rest of the project on this rather than rolling our own solution. I think this is quite an important point that cannot just be dismissed by "we think our way is better". It needs a discussion on llvm-dev about what is wrong about the current approach of the rest of the project, and movement there. I believed that that discussion was predicated on us making this change which is why I wanted to push it through. If that's not the case and this change needs to be part of that discussion, can someone that thinks the current approach of the rest of the project is wrong start that as soon as possible? There's no point in me trying to start it as I don't disagree with the rest of the project.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79507