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

[Frontend][Diagnostics] Improve emitting diagnostic information #5581

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sfzhu93
Copy link
Contributor

@sfzhu93 sfzhu93 commented Jan 11, 2025

Summary

This PR enhances the current implementation for emitting diagnostic remarks by introducing a unified handler in ir.cc. This handler manages diagnostic information more effectively and disables the emission of IRs unless explicitly requested by the user. The MLIR_ENABLE_DIAGNOSTICS environment variable now controls all diagnostic emission settings, accepting one or more values from {warnings, remarks, stacktraces, operations}, separated by commas. Detailed usage instructions are available in the README.

Background

Previously, a new default LLVM SourceManager was configured in nvidia/backend/compiler.py to support remarks, applied in both make_ttgir and make_llir. However, a custom handler already existed in ir.cc, and a more robust design should extend this handler rather than create a new one.

Changes

  • Unified Handler: Inspired by LLVM upstream [PR 117669]([MLIR][mlir-opt] add support for disabling diagnostics llvm/llvm-project#117669), this PR implements a similar custom handler that supports various severity levels. The MLIR_ENABLE_DIAGNOSTICS environment variable now specifies the severity level: warnings for warnings and errors, and remarks for remarks, warnings, and errors.

  • IR Emission Control: By default, the MLIR diagnostic API emits IRs, which can clutter error messages or performance remarks. This PR suppresses IR emission unless explicitly enabled by the user, improving the readability of error messages and performance remarks. Users can specify MLIR_ENABLE_DIAGNOSTICS=remarks,operations to include IR operations in remarks.

  • Stacktraces: Previously, setting MLIR_ENABLE_DIAGNOSTICS=1 enabled all diagnostic information with stacktraces. Now, the stacktraces parameter specifically enables stacktraces. For example, MLIR_ENABLE_DIAGNOSTICS=remarks,operations,stacktraces enables IR operations and stacktraces, displaying all remarks, warnings, and errors.

  • Testing: Updated existing Python tests to verify that combinations of operations and stacktraces are emitted successfully.

Future Work

  • With the new handler in place, there is an opportunity to further enhance the readability of existing warnings and remarks. This will be a focus in future updates.

New contributor declaration

  • I am not making a trivial change, such as fixing a typo in a comment.

  • I have written a PR description following these
    rules.

  • I have run pre-commit run --from-ref origin/main --to-ref HEAD.

  • Select one of the following.

    • I have added tests.
      • /python/test for end-to-end tests
    • This PR does not need a test.
  • Select one of the following.

    • I have not added any lit tests.
    • The lit tests I have added follow these best practices,
      including the "tests should be minimal" section. (Usually running Python code
      and using the instructions it generates is not minimal.)

@sfzhu93 sfzhu93 requested a review from ptillet as a code owner January 11, 2025 21:33
@sfzhu93 sfzhu93 changed the title [Performance Remarks] Improve emitting diagnostic information [WIP][Performance Remarks] Improve emitting diagnostic information Jan 11, 2025
@sfzhu93 sfzhu93 marked this pull request as draft January 11, 2025 21:44
@sfzhu93 sfzhu93 marked this pull request as ready for review January 12, 2025 07:15
@sfzhu93 sfzhu93 changed the title [WIP][Performance Remarks] Improve emitting diagnostic information [Performance Remarks] Improve emitting diagnostic information Jan 12, 2025
@sfzhu93 sfzhu93 marked this pull request as draft January 13, 2025 20:32
@sfzhu93 sfzhu93 force-pushed the migrate-diagnostic-severity-from-llvm branch from bd12964 to 205471f Compare January 13, 2025 20:38
@sfzhu93
Copy link
Contributor Author

sfzhu93 commented Jan 13, 2025

Sorry for pinging you for review - I did a wrong merge on my branch.

@sfzhu93 sfzhu93 force-pushed the migrate-diagnostic-severity-from-llvm branch from 205471f to eea3604 Compare January 13, 2025 23:35
@sfzhu93 sfzhu93 marked this pull request as ready for review January 14, 2025 00:00
@sfzhu93 sfzhu93 changed the title [Performance Remarks] Improve emitting diagnostic information [Frontend][Diagnostics] Improve emitting diagnostic information Jan 14, 2025
Copy link
Collaborator

@manman-ren manman-ren left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up/restructuring! This PR looks good to me.

python/src/ir.cc Show resolved Hide resolved
@sfzhu93 sfzhu93 force-pushed the migrate-diagnostic-severity-from-llvm branch from ab0b297 to 36ddd87 Compare January 24, 2025 07:22
revert some previous changes

update

update

update

update

update

update
@sfzhu93 sfzhu93 force-pushed the migrate-diagnostic-severity-from-llvm branch from 36ddd87 to 2da49ad Compare January 24, 2025 07:26
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