Skip to content

Support performance warning#3922

Merged
manman-ren merged 10 commits intotriton-lang:mainfrom
manman-ren:perf-warning
Jun 19, 2024
Merged

Support performance warning#3922
manman-ren merged 10 commits intotriton-lang:mainfrom
manman-ren:perf-warning

Conversation

@manman-ren
Copy link
Copy Markdown
Collaborator

@manman-ren manman-ren commented May 15, 2024

This commit adds a performance warning for not selecting MMA v3 for tl.dot on Hopper.
For the added test case, we will get:

test-warning.py:24:18: remark: Warning: can't use MMA V3 for the dot op
    c = tl.dot(a, b)
                 ^
test-warning.py:24:18: note: see current operation: %39 = tt.dot %37, %38, %cst, inputPrecision = tf32 :

@manman-ren manman-ren requested a review from ptillet as a code owner May 15, 2024 16:18
@manman-ren manman-ren marked this pull request as draft May 15, 2024 16:18
@manman-ren manman-ren requested review from Jokeren, bertmaher and htyu May 15, 2024 16:22
@Jokeren
Copy link
Copy Markdown
Contributor

Jokeren commented May 15, 2024

Where is this flag coming from "-Wno-perf-warnings"?

@Jokeren
Copy link
Copy Markdown
Contributor

Jokeren commented May 15, 2024

Adds additional warnings obtained when compiling from LLVM to assembly (e.g., register spillage, ptx performance warnings, etc.).

Can you use remark to emit ptx warnings? Maybe not?

@ptillet
Copy link
Copy Markdown
Collaborator

ptillet commented May 16, 2024

python/tutorials/test-warning.py

We shouldn't have tests as tutorials :) Could you create a unit test that exercises the new codepaths and checks for the output on std::cerr?

@manman-ren
Copy link
Copy Markdown
Collaborator Author

Thanks for the comments! As described in the summary:

  • Where should we add the test case to detect the diagnostics? I can add a mlir test case with -verify-diagnostics and expected-remark. Right now, I am adding a test case under python/tutorial temporarily we will need a way to verify diagnostics are emitted at the right source line.
    @ptillet Yeah I added to tutorial temporarily for discussion, I will move to the unit directory, which will be tested via pytest and I will try to figure out how to check against stdout there. If you have a test case that does similar thing, that will be great!
  • Do we want Remarks or Warnings? For llvm backend, remarks may work better.
  • Support warning flags or use env variables? It is not clear to me how to support warning flags when building the .py source code.
    @Jokeren If we want to go with warning flags, I need to figure out how to support -Wno-perf-warnings when building a py code. Again if anyone has any pointer, that will be great.

Can you use remark to emit ptx warnings? Maybe not?

You mean warnings from ptxas, not sure how to get them. But we can emit remarks when lowering from llvm to ptx.

CC @joker-eph: if you have any suggestion to some of the questions, that will be great, Thanks!

@Jokeren
Copy link
Copy Markdown
Contributor

Jokeren commented May 16, 2024

You mean warnings from ptxas, not sure how to get them. But we can emit remarks when lowering from llvm to ptx.

My worry was that some warnings might be emitted from ptx to sass. I'm OK with either marks or warnings. I think marks actually seem more informative.

@joker-eph
Copy link
Copy Markdown
Contributor

You mean warnings from ptxas, not sure how to get them.

You could capture the output of ptxas and regex/pattern match the output to catch these and report them as remarks?

@ptillet
Copy link
Copy Markdown
Collaborator

ptillet commented May 17, 2024

Where should we add the test case to detect the diagnostics

test/unit/warnings.py sounds like a good place

I will try to figure out how to check against stdout there

Just to be clear, this should check against stderr

Comment thread python/test/unit/test_perf_warning.py Outdated
out_messages = out_capture.getvalue()
sys.stderr = sys.__stderr__
sys.stdout = sys.__stdout__
print(error_messages)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This test case is not working currently. I haven't figured out how to capture the stderr in a string yet. Tried to redirect stderr, but it may be captured by pytest? The message shows:
--------------------------------------------------------------------------- Captured stderr call ---------------------------------------------------------------------------
python/test/unit/test_warning.py:21:18: remark: Warning: can't use MMA V3 for the dot op
c = tl.dot(a, b)
^
python/test/unit/test_warning.py:21:18: note: see current operation: %46 = tt.dot ...

Will clean up this once it is solved. @htyu Any suggestion here? Thanks!

@manman-ren
Copy link
Copy Markdown
Collaborator Author

@ptillet About using warning flags -Wno-perf-warnings or -Werror, I feel it is not supported in the python workflow "python test.py", right? So is the suggestion about supplying the flags for triton-opt and still use env variables for the python workflow?
CC @joker-eph in case Mehdi has some comments.

Comment thread third_party/nvidia/backend/compiler.py Outdated
srcMgr = llvm.source_mgr()
diag = ir.source_mgr_diag(srcMgr, mod.context)
mod.context.printOpOnDiagnostic(True)
#mod.context.printStackTraceOnDiagnostic(True)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: remove the comment?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Actually I think instead we may want to add an enable_remarks API similar to enable_debug:

.def("enable_debug",

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@htyu Thanks for the review! Sorry for the delay. I tried to wrap things in enable_remarks:
as part of Context

   py::class_<MLIRContext>(m, "context", py::module_local())
       .def(py::init<>())
+      .def("enable_remark",
+           [](MLIRContext &self) {
+             auto srcMgr = llvm::SourceMgr();
+             auto diag = SourceMgrDiagnosticHandler(srcMgr, &self);
+             self.printOpOnDiagnostic(true);
+           })

or as part of PassManager

   py::class_<PassManager>(m, "pass_manager", py::module_local())
       .def(py::init<MLIRContext *>())
+      .def("enable_remark",
+           [](PassManager &self) {
+             auto *context = self.getContext();
+             auto srcMgr = llvm::SourceMgr();
+             auto diag = SourceMgrDiagnosticHandler(srcMgr, context);
+             context->printOpOnDiagnostic(true);
+           })

Then call from make_ttgir:
with either
mod.context.enable_remark()
or
pm.enable_remark()

None of these worked. I think it is due to the liveness of SourceMgr and SourceMgrDiagnosticHandler.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for giving it a shot. Currently changes look good to me.

@manman-ren manman-ren marked this pull request as ready for review June 13, 2024 21:26
@manman-ren
Copy link
Copy Markdown
Collaborator Author

The tests are failing. I need to only enable the test for H100 with the env variable.

cluster_info.clusterDimY = opt.cluster_dims[1]
cluster_info.clusterDimZ = opt.cluster_dims[2]
# Set up Diagnostic
if os.environ.get("MLIR_ENABLE_REMARK", "0") == "1":
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you document in README?

@manman-ren manman-ren merged commit 75b0321 into triton-lang:main Jun 19, 2024
bertmaher pushed a commit to bertmaher/triton that referenced this pull request Dec 10, 2024
This commit adds a performance warning for not selecting MMA v3 for
tl.dot on Hopper.
For the added test case, we will get:
```
test-warning.py:24:18: remark: Warning: can't use MMA V3 for the dot op
    c = tl.dot(a, b)
                 ^
test-warning.py:24:18: note: see current operation: %39 = tt.dot %37, %38, %cst, inputPrecision = tf32 :
```
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.

6 participants