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

[DEBUG] Only print out diagnostic messages if an environment variable is set #3147

Merged
merged 2 commits into from
Feb 19, 2024

Conversation

gflegar
Copy link
Collaborator

@gflegar gflegar commented Feb 19, 2024

This prevents crashes in test_core.py due to too many diagnostics emitted in llvm/llvm-project#78228 It should also speed up compile times, as we can use multithreading, and avoid handling diagnostic messages.

python/src/ir.cc Outdated Show resolved Hide resolved
This prevents crashes in test_core.py due to too many diagnostics emitted in llvm/llvm-project#78228
It should also speed up compile times, as we can use multithreading, and avoid handling diagnostic messages.
@Jokeren Jokeren changed the title Only print out diagnostic messages if an environment variable is set [DEBUG] Only print out diagnostic messages if an environment variable is set Feb 19, 2024
Copy link
Collaborator

@ThomasRaoux ThomasRaoux left a comment

Choose a reason for hiding this comment

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

Thanks!

@ThomasRaoux ThomasRaoux merged commit 3ffb5a2 into triton-lang:main Feb 19, 2024
4 checks passed
Jokeren pushed a commit that referenced this pull request Feb 19, 2024
… is set (#3147)

This prevents crashes in test_core.py due to too many diagnostics
emitted in llvm/llvm-project#78228 It should
also speed up compile times, as we can use multithreading, and avoid
handling diagnostic messages.
@jlebar
Copy link
Collaborator

jlebar commented Feb 19, 2024

After some debugging on our side, I am not sure that this patch does what was intended.

  • If you don't call context->getDiagEngine().registerHandler([](Diagnostic &diag) {, MLIR seems to register one by default that prints to stdout or stderr, which is identical to what we do when we do register a handler, so this has no effect either way.
  • context->printOpOnDiagnostic(true); is the default, so this has no effect either way.
  • context->printStackTraceOnDiagnostic(true); is not the default, but setting it doesn't seem to have an effect as far as we can tell.
  • We observe that the diagnostics printed out when Triton fails seem to be identical with and without this patch (with no envvar set).

Therefore the only effect this patch seems to have is enabling multithreading unless you set the new envvar.

We are happy that the diagnostics are the same with and without this patch, at least in the way we use Triton. If you want to make a patch to actually print fewer diags, that should probably be disabled by default, and you can enable it on your side.

@gflegar
Copy link
Collaborator Author

gflegar commented Feb 20, 2024

Thanks for looking into it. That's quite interesting, I just double checked if I missed the printouts in our output after the patch, and I can clearly see them before it, and they disappeared afterwards.

I see two possibilities why that happened: either it's because we are using a newer version of LLVM, and the default MLIR behavior changed (you could try patching in #3150 to try it out), or it has something to do with how the pytest runner works (it's a bit of a mystery to me TBH) - e.g., maybe enabling multithreading again somehow prevents the crash, and pytest only prints out stdout / stderr if the test fails.

@gflegar
Copy link
Collaborator Author

gflegar commented Feb 20, 2024

According to the latest comments in #3150 it seems that this is doing something even upstream with a newer version of LLVM.

binarman pushed a commit to binarman/triton that referenced this pull request Apr 2, 2024
… is set (triton-lang#3147)

This prevents crashes in test_core.py due to too many diagnostics
emitted in llvm/llvm-project#78228 It should
also speed up compile times, as we can use multithreading, and avoid
handling diagnostic messages.
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.

4 participants