f18
f18 copied to clipboard
flang-compiler
[LLVMify F18] Use llvm_unreachable for unreachable code cases
llvm_unreachable expands to an assert in debug mode and a compiler optimization hint abstracted across the supported compilers in release mode. This allows additional compiler optimizations based on this assumption.
We should replace our uses of assert(false), DIE("unreachable") and similar with llvm_unreachable to take advantage of this.
There are no instances of assert(false) or CHECK(false) in f18. There's 9 instances of die("unreachable") or DIE("unreachable"), all in the same file. I recommend replacement with a macro, which can be defined alongside the macro CRASH_NO_CASE.
Why define our own macro when something suitable already exists elsewhere in the project?
Because, as mentioned above, you'll probably also want to modify CRASH_NO_CASE.
llvm_unreachable will always print the filename and the line number of the problematic location.
llvm_unreachable("..") is a drop in replacement for DIE(".."). so different cases like CRASH_NO_CASE, SWITCH_COVERS_ALL_CASES and CHECK can leverage llvm_unreachable(..) similar to DIE("..).
But Fortran::common::die accepts varargs. This honors the format specifiers. In all the case this function will not print the filename and the line number. Ex: common::die("missing case to fold intrinsic function %s", name.c_str()) The error we get is: fatal internal error: missing case to fold intrinsic function xyz
Should we replace this function also to use llvm_unreachable or can we have f18 specific implementation for this scenario?
Either ways, for the above case we need to have f18 specific implementation to honor format specifiers, as llvm_unreachable does not honor format specifier. Please advice. Correct me if my understanding is incorrect.
Hi Kiran, thanks for looking at this. At a glance I think we can replace our die function with something like the following (I haven't tried to compile this code so it may be nonsense):
template <typename Args...>
[[noreturn]] void die(const char* c, Args &... args) {
std::string buf;
llvm::raw_string_ostream ss {buf};
buf << llvm::format(c, args...);
llvm_unreachable(buf.str());
}
and then get rid of our DIE macro. Do you think something like this could work?
As regards the other point, I can't see why the file and line information wouldn't always be useful, so I don't think we need the distinction between the two.
Thanks @DavidTruby. Your code was almost perfect. :-) I have replaced DIE with llvm_unreachable and raised a pull request.
We must not conflate assertion checks with unreachable code in llvm because llvm makes a distinction between assert and llvm_unreachable.
See https://llvm.org/docs/CodingStandards.html#id42
Use llvm_unreachable to mark a specific point in code that should never be reached.
LLVM disables the assert and llvm_unreachable checks in RELEASE builds. As far as I know, re-enabling assert in a RELEASE build requires a bit of ingenuity.
I think it is better not to use assert directly in the source, but instead to have a function call or different macro as an intermediary to assist debugging of RELEASE builds.
I believe CMake's RelWithDebugInfo build type leaves assets on? That should help with debugging optimised builds if so.
The advantage of using llvm_unreachable and assert is they both use compiler specific macros/intrinsics to signal Undefined Behaviour. This allows extra compiler optimisations in some cases (e.g. not bothering to generate code for an unreachable case, as an obvious example)
What I said seems to be untrue, RelWithDebInfo still has -DNDEBUG. However, LLVM does have an LLVM_ENABLE_ASSERTIONS variable in cmake to force both assert and llvm_unreachable on in any build configuration, and that can be used in conjunciton with Release builds.
The idea is to enable flang assertion checking without enabling everything that controlled by NDEBUG.
Also, unless potential undefined behavior is intended, we ought to leave assertions and checks for unreachable code enabled all the time. We might consider adopting something like Rust's debug_assert.
Doesn't debug_assert in Rust do the same thing as assert here? As in, it gets disabled in optimised/release builds?
It's not so much that potential UB is intended as it is that it allows extra interesting optimisations for the compiler to take advantage of. I don't see a reason we wouldn't want these optimisations to occur in Release/optimised builds?
Yes, Rust's debug_assert does what std::assert does and Rust's assert is checked for both debug and release builds.
Assuming that we want the assertion checks, what the issue with optz?
CHECK and DIE are intended to be enabled in release builds. std::assert and llvm_unreachable are available for when you want them disabled in release builds.
To change existing use of the former to the latter we need to make the case that the performance and size benefits are greater than the debugging benefits.
Do we need users to be able to use these for debugging? Because, as developers we could just build with -DLLVM_ENABLE_ASSERTIONS and test with that in release mode (which is what we already do in our CI downstream for what it's worth)
Do we need users to be able to use these for debugging?
We want them enabled so that when users hit them they get a clear indication that it is an internal error and information that is helpful to us about where it happened.
Ah, I see what you mean now. I agree that there's a difference between assert and unreachable code, and also that there's a difference between something we want to use assert for and an ICE that a user might hit. However, I think that a lot of the cases in the PR that @kiranktp had up before fall into the former category.
We're currently lumping both "function preconditions that we expect tests to catch", "actually unreachable code" and "Possible ICEs" into the same category with the CHECK/DIE macros. I think rather we should consider using assert for the first category, llvm_unreachable for the second and some more robust mechanism for reporting ICEs for the third.
Perhaps we should look at how clang reports ICEs? It provides nice stack traces and more diagnostic info for those cases.
CHECK and DIE are intended to be enabled in release builds
LLVM has report_fatal_error for this, does it match your "DIE" use case?
We want them enabled so that when users hit them they get a clear indication that it is an internal error and information that is helpful to us about where it happened.
Anyone can ship an LLVM toolchain with LLVM_ENABLE_ASSERTIONS=ON ;)
So, LLVM's philosophy here is:
- if this is an invariant that represents a programmer error (something that should be fixed by changing some source code - either inside or (if using LLVM as a library) outside LLVM) if it fails - use assert and llvm_unreachable (llvm_unreachable is described in the LLVM developer guide as "a better assert(false)")
- if this is a runtime error meant to be handled by code/users (or users that are code - if they're using parts of LLVM as a library) it should use the usual error handling such as llvm::Error
- report_fatal_error is sort of a dodgy stop-gap. It's for errors where someone finds it too onerous to do (2) but they think it might actually happen - but it hard stops the program. This isn't appropriate error handling for a library which LLVM and its subprojects strive to be
This is something I would have some pretty strong feelings about with regard to flang merging into the LLVM project: Please assert/unreachable your preconditions. If users experience crashes, having report_fatal_error won't make things any better for them - compiler engineers will take their reproduction and use a +Asserts build (as mentioned - it doesn't require any particular ingenuity to enable them - LLVM_ENABLE_ASSERTIONS=ON - yes, anyone developing code inside LLVM or using LLVM as a library should be using a build with assertions enabled) to get more info as the first step in debugging.
@dwblaikie thanks for your advice here! I am willing to commit myself to take a much deeper look at error handling as a first priority post-merge; but I think it'll be much easier to do then than before merging. Once we're on the same infrastructure it will be easier to attract reviewers from the whole LLVM community, and getting changes committed in line with what that community wants should be a lot easier as a result. If I commit myself to that, will this be a blocker for you with regards to the merge?
The state of the code is less important to me than the agreement of the f18/flang community - it's something I think as a community it should be clear agreement going into this.
There was an objection to replacing unreachable code with assert instead of llvm_unreachable. I believe we're on the same page about llvm_unreachable for things like unhandled cases.
There's still discussion about how to represent runtime checks that we want to always be enabled, with or without NDEBUG. NDEBUG is a big hammer that I would prefer be independent of low-cost runtime checks, like null pointer checks.
I'm fine with using NDEBUG and std::assert for checks that are expensive or otherwise undesirable when the compiler is running.
There was an objection to replacing unreachable code with assert instead of llvm_unreachable. I believe we're on the same page about llvm_unreachable for things like unhandled cases.
There's still discussion about how to represent runtime checks that we want to always be enabled, with or without NDEBUG. NDEBUG is a big hammer that I would prefer be independent of low-cost runtime checks, like null pointer checks.
Yeah, that would be a divergence from the LLVM project that I wouldn't be comfortable with. LLVM asserts liberally for even low-cost checks like null pointers & I believe that's the right call. (LLVM does have a layer beyond asserts - "expensive checks" for the really expensive things, but even enabling the non-expensive checks is more expensive than you'd want in release builds)
This is the LLVM style guide's wording on assertions: https://llvm.org/docs/CodingStandards.html#assert-liberally - the examples show fairly cheap tests.
@sscalpone at least according the to the C++ standard, NDEBUG does nothing other than enable the assert macro. What other behaviour are you expecting from it that makes it a big hammer?
There's still discussion about how to represent runtime checks that we want to always be enabled, with or without NDEBUG. NDEBUG is a big hammer that I would prefer be independent of low-cost runtime checks, like null pointer checks.
Yeah, that would be a divergence from the LLVM project that I wouldn't be comfortable with.
I think I'm missing something here. You're not comfortable with flang containing runtime checks in release builds? Why?
There's still discussion about how to represent runtime checks that we want to always be enabled, with or without NDEBUG. NDEBUG is a big hammer that I would prefer be independent of low-cost runtime checks, like null pointer checks.
Yeah, that would be a divergence from the LLVM project that I wouldn't be comfortable with.
I think I'm missing something here. You're not comfortable with flang containing runtime checks in release builds? Why?
Runtime checks for invariants, yes - because it's not consistent with the LLVM style guide/way of writing code. (& because I agree with it - and I think it's important to not treat invariant violations the same as user errors - in part because testing is very different, you don't write test cases to demonstrate invariant violations, but you should have test cases for every user error, for instance)
What other behaviour are you expecting from [NDEBUG] that makes it a big hammer?
A lot of code uses NDEBUG to disable runtime checks. There are over 1300 instances in llvm-project. Not all of these are low-cost.
The Rust language deals with the issue head on: asserts are checked in debug and release builds, and cannot be disabled; debug_assert is not enabled in release builds by default. I'm not suggesting that llvm adopt these semantics for assert, but instead suggesting that flang define its own always-on assertions.
I think I'm missing something here. You're not comfortable with flang containing runtime checks in release builds? Why?
Runtime checks for invariants, yes - because it's not consistent with the LLVM style guide/way of writing code.
So the requirement is for undefined behavior rather than abort with a message when an invariant is not satisfied? That makes no sense.
Makes a fair bit of sense to the rest of the LLVM project, and I'd not be comfortable with flang being different in this regard.
The requirement is to be able to opt-in to invariant checking, not having it always-on.