Page MenuHomePhabricator

[mlir][llvm] Use the tablegen error handling (NFC).
ClosedPublic

Authored by gysit on Oct 17 2022, 12:03 AM.

Details

Summary

Use PrintError to extend the error message with location information in
LLVMIRConversionGen.cpp. Additionally, promote potentially user facing
error messages from assertions to real errors.

Diff Detail

Event Timeline

gysit created this revision.Oct 17 2022, 12:03 AM
gysit requested review of this revision.Oct 17 2022, 12:03 AM

It looks like the point of a custom method was to exit more gracefully than abort. Can you comment on why harder failures are desired?

Also, this turns both assertions and user-oriented errors into the same kind. These have different roles: assertions are meant for mlir-tablegen developers while errors are meant for its clients. If an assertion can be triggered by user input, we should check its condition and either fix the code around or promote it to a user-visible error. But this is orthogonal to how the errors are emitted.

It looks like the point of a custom method was to exit more gracefully than abort. Can you comment on why harder failures are desired?

I thought the PrintFatalError method is the standard way to handle errors in such tablegen code generation files? It is quite nice in the sense that it adds location info to the error message.

The reason why I started this the change is that asserts did not trigger for me (I have CMAKE_BUILD_TYPE:STRING=Debug and LLVM_ENABLE_ASSERTIONS:BOOL=ON)? It may well be that something with my configuration is wrong though.

There is also a PrintError maybe that one would be a better choice?

It looks like the point of a custom method was to exit more gracefully than abort. Can you comment on why harder failures are desired?

I thought the PrintFatalError method is the standard way to handle errors in such tablegen code generation files? It is quite nice in the sense that it adds location info to the error message.

The reason why I started this the change is that asserts did not trigger for me (I have CMAKE_BUILD_TYPE:STRING=Debug and LLVM_ENABLE_ASSERTIONS:BOOL=ON)? It may well be that something with my configuration is wrong though.

There is also a PrintError maybe that one would be a better choice?

It'd be nice to stray away from writing PrintFatalError in any new tablegen code that we write. It's really bad for error handling, and we should really prefer cleaner exits.

It looks like the point of a custom method was to exit more gracefully than abort. Can you comment on why harder failures are desired?

I thought the PrintFatalError method is the standard way to handle errors in such tablegen code generation files? It is quite nice in the sense that it adds location info to the error message.

It is part of the "old" LLVM error handling, we don't want it in new code. Adding the location is pretty much the only benefit it has.

The reason why I started this the change is that asserts did not trigger for me (I have CMAKE_BUILD_TYPE:STRING=Debug and LLVM_ENABLE_ASSERTIONS:BOOL=ON)? It may well be that something with my configuration is wrong though.

This sounds strange. Maybe try running under the debugger. Also check that the same compiler flags are getting used for tools and for libraries, maybe some cross-compilation related weirdness is involved.

There is also a PrintError maybe that one would be a better choice?

It looks more reasonable, at least it does not abort indiscriminately. I'd keep it wrapped in emitError so it plays nicely with the error return flow. Unless @rriddle has a better idea.

gysit added a comment.Oct 17 2022, 2:22 AM

This sounds strange. Maybe try running under the debugger. Also check that the same compiler flags are getting used for tools and for libraries, maybe some cross-compilation related weirdness is involved.

The assertion not triggering was my bad! I had optimized tablegen builds enabled...

Let's stick to emitError then maybe extended a little bit with some location info printing. I will also promote some of the assertions to errors since they seem relevant for tablegen users (I suspect many of them will build with optimized tablegen).

gysit updated this revision to Diff 468160.Oct 17 2022, 4:27 AM

Address comments.

ftynse accepted this revision.Oct 17 2022, 4:28 AM
This revision is now accepted and ready to land.Oct 17 2022, 4:28 AM
gysit edited the summary of this revision. (Show Details)Oct 17 2022, 4:28 AM
This revision was automatically updated to reflect the committed changes.