Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-enable diagnostic error/warning printing #3020

Conversation

AlexandreEichenberger
Copy link
Collaborator

@AlexandreEichenberger AlexandreEichenberger commented Nov 27, 2024

In the past, emitError(...) printed messages to stderr, but now we have to install a diagnostic error handler. So I added the necessary code, as otherwise we do not get error messages anymore.

Also, in onnx-mlir diagnostics, we issued an assert when a range was badly formed (max<min), but since that can be the reason for an emitError to begin with, it also prevented an error message to be printed. So now the range can be badly formed, we can test for it, and just adding an extra error/warning message.

… also prevented printing

Signed-off-by: Alexandre Eichenberger <[email protected]>
@@ -743,7 +743,7 @@ class LoopOpRewriteMaxTripCountPattern : public OpRewritePattern<ONNXLoopOp> {
bool isDefinedByIntegerConstantOp(Value v) const {
if (mlir::isa<BlockArgument>(v))
return false;
Operation *definingOp = v.getDefiningOp();
// Operation *definingOp = v.getDefiningOp();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was unused.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you remove it completely?

@AlexandreEichenberger
Copy link
Collaborator Author

Note we get a warning now when emitting LLVM because some of our functions have an non-MLIR approved / recognized attribute. The warning is

Unhandled parameter attribute 'onnx.name'

I verified that it is in the function

llvm.func @main_graph_test_add(%arg0: !llvm.ptr, %arg1: !llvm.ptr, %arg2: i64, %arg3: i64, %arg4: i64, %arg5: i64, %arg6: i64, %arg7: i64, %arg8: i64, %arg9: !llvm.ptr, %arg10: !llvm.ptr, %arg11: i64, %arg12: i64, %arg13: i64, %arg14: i64, %arg15: i64, %arg16: i64, %arg17: i64) -> (!llvm.struct<(ptr, ptr, i64, array<3 x i64>, array<3 x i64>)> {onnx.name = "sum"}) attributes {llvm.emit_c_interface} {
...

Since its probably ok, I currently disable the diagnostic reporting in the LLVM lowering unless the -v (Verbose) mode is requested.

Copy link
Collaborator

@tungld tungld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@@ -743,7 +743,7 @@ class LoopOpRewriteMaxTripCountPattern : public OpRewritePattern<ONNXLoopOp> {
bool isDefinedByIntegerConstantOp(Value v) const {
if (mlir::isa<BlockArgument>(v))
return false;
Operation *definingOp = v.getDefiningOp();
// Operation *definingOp = v.getDefiningOp();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you remove it completely?

@AlexandreEichenberger AlexandreEichenberger merged commit e801b36 into onnx:main Dec 4, 2024
7 checks passed
@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #16051 [push] Re-enable diagnostic err... started at 12:15

@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #15078 [push] Re-enable diagnostic err... started at 12:31

@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #16049 [push] Re-enable diagnostic err... started at 11:15

@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #16049 [push] Re-enable diagnostic err... aborted after 1 hr 19 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #16051 [push] Re-enable diagnostic err... aborted after 1 hr 19 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #15078 [push] Re-enable diagnostic err... aborted after 1 hr 19 min

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants