[Comgr] Replace llvm_unreachables with graceful errors#1635
[Comgr] Replace llvm_unreachables with graceful errors#1635chinmaydd wants to merge 1 commit intoROCm:amd-mainfrom
Conversation
| } | ||
|
|
||
| llvm_unreachable("invalid language"); | ||
| return "<unknown>"; |
There was a problem hiding this comment.
Could clean these up to be AMD_COMGR_LANGUAGE_UNKNOWN maybe ?
There was a problem hiding this comment.
In theory, this really is unreachable though right? The error message would probably be more accurate if it said something like "it shouldn't be possible to get here"
There was a problem hiding this comment.
What do you think about adding a failing assertion?
Maybe a call to something like:
static void assert_unreachable() __attribute__((cold)) {
assert(false && "Unreachable");
}Then developer builds could catch early that something undefined is happening.
There was a problem hiding this comment.
In theory, this really is unreachable though right?
I hear you, but with most AMD compiler and runtime components open-source one could develop custom forks with other "custom" language support. I think we should more generally have an infrastructure that does not crash by default. Another option would be to add a default case and then clean up the unreachable. I'm okay with that.
Then developer builds could catch early that something undefined is happening.
Do we test with assertions on in our CQE testing phases ? If so, then I can get behind that.
|
!PSDB |
|
PSDB Build Link: http://mlse-bdc-20dd129:8065/#/builders/11/builds/25 |
5 participants
Comgr should fail gracefully through all code paths.
This PR is a first step towards that effort.