Address outstanding code review comments #1994

mirza-halilcevic · 2025-09-19T13:31:45Z

Continued Introduce new quick tune lists based on Tier1 configs and separated by architecture #1907
Resolves https://github.com/ROCm/rocMLIR-internal/issues/1967

mlir/lib/Dialect/Rock/Tuning/GridwiseGemmParams.cpp

umangyadav

Looks good, but see if you can address comment about auto generating some of the stuffs.

dhernandez0 · 2025-10-21T08:21:17Z

mlir/lib/Dialect/Rock/Tuning/GridwiseGemmParams.cpp

+      return ArrayRef<ParamsType>(it->second.first, it->second.second);
+    }
+
+    auto archFamily = getArchName(arch).drop_back(2).str();


do we want to always drop two? Take into account gfx9 typically has 3 numbers only (gfx950, gfx942, ...)

Good point, I changed the fallback lookup significantly. Let me know what you think.

dhernandez0 · 2025-10-21T08:22:44Z

mlir/lib/Dialect/Rock/Tuning/GridwiseGemmParams.cpp

+    for (const auto &entry : table) {
+      if (entry.first.find(archFamily) != std::string::npos &&
+          entry.first.find(makeSuffix(op, dataType)) != std::string::npos) {
+        fallbackKey = std::max(fallbackKey, entry.first);


what does std::max() of two strings do? take the longest string?

Compare lexicographically. The idea is that it would give me the latest arch string, e.g. gfx1201 is preferred to gfx1200.

dhernandez0 · 2025-10-21T08:23:56Z

mlir/lib/Dialect/Rock/Tuning/GridwiseGemmParams.cpp

+      llvm_unreachable("Invalid architecture string");
+    }
+    auto remaining = arch.substr(gfxPos);
+    auto endPos = remaining.find_first_not_of("0123456789a", 3);


there's gfx1030w apparently as well. Please can you check all possible codes somewhere? (maybe llvm)?

It now looks for the first non-alphanumeric. That should be flexible enough yet not too verbose.

dhernandez0 · 2025-10-21T08:26:18Z

mlir/lib/Dialect/Rock/Tuning/GridwiseGemmParams.cpp

+    llvm::raw_string_ostream os(dataTypeStr);
+    os << dataType;
+
+    if (dataTypeStr == "bf16") {


can you add a comment about why we do this? we are assuming f16/bf16 have the same optimal perfconfigs, right? same for f8

nit: consider doing dataType.getIntOrFloatBitWidth() == 8 && isa < FloatType > (dataType), instead of matching strings. I think that's more robust for future datatypes.

Great suggestion.

We don't tune for bf16, so the f16 lists are used instead. It is an assumption.

After looking more into it, there seem to be more edge cases who went under the radar with the previous approach. Previously, because of the if-else chains, the selection of the quick tune lists was much less strict, and the else statements were acting like catch-alls. For example, the gfx1200 quick tune list would be selected for a gfx942 non-accel problem, because we don't have any non-accel list for gfx942. A lot of tests fail now because of this. Also, for any non-accel case we would return f32 lists, because we don't have non-accel lists for other types. The non-accel cases seem to happen because of the following line:

rocMLIR/mlir/tools/rocmlir-gen/rocmlir-gen.cpp

Line 4350 in 68727b9

convGenerator.flipAccel();

Any thoughts? On one hand it is incorrect to return a list for the wrong arch and/or data type, but on the other hand we have tests where this happens. Maybe it doesn't matter that much in these non-accel cases, and we can just fall back to any list?

@umangyadav Can you also chime in?

mlir/lib/Dialect/Rock/Tuning/GridwiseGemmParams.cpp

digits of identifier for gfx1* architectures.

getArchName so it handles gfx90a.

Copilot

Pull Request Overview

This PR refactors tuning parameter lookup in the Rock dialect by introducing a template-based ParamLookupTable class to replace hardcoded architecture-specific logic. The changes also improve the quickTuningGen.py script to support dynamic lookup table generation and fix various parameter naming inconsistencies.

Replaces large switch statements in C++ with a centralized lookup table mechanism
Adds dynamic lookup table generation and insertion logic to the Python tuning script
Standardizes parameter naming conventions (e.g., no_splitk → no_splitk throughout)

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
mlir/lib/Dialect/Rock/Tuning/GridwiseGemmParams.cpp	Introduces `ParamLookupTable` template class to replace hardcoded parameter lookups with table-driven approach
mlir/utils/performance/analysis/quickTuningGen.py	Adds lookup table generation, refactors parameter name logic, fixes naming inconsistencies (splitK → splitk)
mlir/include/mlir/Dialect/Rock/Tuning/QuickTuningPerfconfigs.inc	Adds lookup table entries for NonAccel and Accel parameter mappings
mlir/include/mlir/Dialect/Rock/Tuning/GridwiseGemmParams.h	Adds friend declarations for `ParamLookupTable` to access private static members
mlir/utils/performance/tuningRunner.py	Adds `index=False` parameter to CSV export for cleaner debug output
mlir/test/Dialect/Rock/affix_tuning_params.mlir	Updates test expectations with new tuning parameters

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-10-29T14:25:19Z

mlir/utils/performance/analysis/quickTuningGen.py

        replacment = f'{begin_marker}\n{new_content}\n{end_marker}'
        content = pattern.sub(replacment, content)


Corrected spelling of 'replacment' to 'replacement'.

Suggested change

replacment = f'{begin_marker}\n{new_content}\n{end_marker}'

content = pattern.sub(replacment, content)

replacement = f'{begin_marker}\n{new_content}\n{end_marker}'

content = pattern.sub(replacement, content)

Copilot · 2025-10-29T14:25:19Z

mlir/utils/performance/analysis/quickTuningGen.py

+        with open(file_path, 'r') as file:
+            content = file.read()
+
+        pattern = re.compile(f'{mapping}', re.DOTALL)


The regex pattern is treating mapping as a literal regex pattern instead of escaping it. This will fail if mapping contains special regex characters like brackets or braces. Use re.escape(mapping) to treat it as a literal string.

Suggested change

pattern = re.compile(f'{mapping}', re.DOTALL)

pattern = re.compile(re.escape(mapping), re.DOTALL)

Copilot · 2025-10-29T14:25:20Z

mlir/utils/performance/analysis/quickTuningGen.py

-    usage: quickTunerGen.py [-h] --input-dir INPUT_DIR --op {gemm,conv} [--th TH] --arch ARCH [--update] [--no-splitK]
-    usage exsample: python3 quickTuningGen.py --input-dir tunedData --op conv --arch gfx90a --update --no-splitK
+    usage: quickTunerGen.py [-h] --input-dir INPUT_DIR --op {gemm,conv} [--th TH] --arch ARCH [--update] [--no-splitk]
+    usage exsample: python3 quickTuningGen.py --input-dir tunedData --op conv --arch gfx90a --update --no-splitk


Corrected spelling of 'exsample' to 'example'.

Suggested change

usage exsample: python3 quickTuningGen.py --input-dir tunedData --op conv --arch gfx90a --update --no-splitk

usage example: python3 quickTuningGen.py --input-dir tunedData --op conv --arch gfx90a --update --no-splitk

Copilot · 2025-10-29T14:25:20Z

mlir/utils/performance/analysis/quickTuningGen.py

                        default=False,
                        action='store_true',
-                        help='Removing the spliK factor from the generated list')
+                        help='Removing the Split-K factor from the generated list')


[nitpick] Help text uses present participle 'Removing' instead of imperative form. Change to 'Remove the Split-K factor from the generated list' for consistency with typical argparse help conventions.

Suggested change

help='Removing the Split-K factor from the generated list')

help='Remove the Split-K factor from the generated list')

mlir/lib/Dialect/Rock/Tuning/GridwiseGemmParams.cpp

umangyadav · 2025-10-29T14:31:51Z

mlir/lib/Dialect/Rock/Tuning/GridwiseGemmParams.cpp

+  // Find the lexicographically closest relative
+  static std::string findFallback(const std::string &target) {


Can you add some docstring on what this is trying to find ?

mlir/lib/Dialect/Rock/Tuning/GridwiseGemmParams.cpp

umangyadav · 2025-10-29T15:01:36Z

Would it be possible to add some tests ?

dhernandez0 reviewed Sep 19, 2025

View reviewed changes

umangyadav approved these changes Sep 19, 2025

View reviewed changes

mirza-halilcevic force-pushed the quick_tune_code_review branch from ff2eb71 to fecd61a Compare October 21, 2025 00:31

mirza-halilcevic marked this pull request as ready for review October 21, 2025 00:32

mirza-halilcevic requested a review from causten as a code owner October 21, 2025 00:32

mirza-halilcevic force-pushed the quick_tune_code_review branch from 77f8a26 to 71b0b00 Compare October 21, 2025 08:16

dhernandez0 reviewed Oct 21, 2025

View reviewed changes

mlir/lib/Dialect/Rock/Tuning/GridwiseGemmParams.cpp Outdated Show resolved Hide resolved

mirza-halilcevic force-pushed the quick_tune_code_review branch 3 times, most recently from 3d29f0f to 3bbfa38 Compare October 25, 2025 16:04

mirza-halilcevic requested review from dhernandez0 and umangyadav October 25, 2025 16:13

mirza-halilcevic force-pushed the quick_tune_code_review branch 3 times, most recently from 6c389c2 to c6f386d Compare October 27, 2025 16:13

mirza-halilcevic added 10 commits October 29, 2025 07:59

Address outstanding review comments.

16460a9

Autogenerate lookup table for quick tuning lists and remove last two

e0b7770

digits of identifier for gfx1* architectures.

Improve scalability of quick tuning lists.

a0b34f3

Replace std::map with std::unordered_map for lookup table and fix

7c2331f

getArchName so it handles gfx90a.

Use f16 quick tuning list in case of bf16 type.

a9efc6d

Fix formatting.

71014d1

Improve fallback lookup and address code review comments.

c70658b

Fallback to any f32 list for non-accel problems.

bd5e8ed

Simplify and improve fallback lookup.

4cee34e

Fix windows build.

a20b5da

mirza-halilcevic force-pushed the quick_tune_code_review branch from 2e174d0 to a20b5da Compare October 29, 2025 13:00

Avoid generating index in debug tuning files.

db2ba72

umangyadav requested a review from Copilot October 29, 2025 14:18

Copilot AI reviewed Oct 29, 2025

View reviewed changes

umangyadav reviewed Oct 29, 2025

View reviewed changes

mlir/lib/Dialect/Rock/Tuning/GridwiseGemmParams.cpp Show resolved Hide resolved

umangyadav reviewed Oct 29, 2025

View reviewed changes

mlir/lib/Dialect/Rock/Tuning/GridwiseGemmParams.cpp Show resolved Hide resolved

dhernandez0 approved these changes Oct 29, 2025

View reviewed changes

		replacment = f'{begin_marker}\n{new_content}\n{end_marker}'
		content = pattern.sub(replacment, content)

	pattern = re.compile(f'{mapping}', re.DOTALL)
	pattern = re.compile(re.escape(mapping), re.DOTALL)

	usage exsample: python3 quickTuningGen.py --input-dir tunedData --op conv --arch gfx90a --update --no-splitk
	usage example: python3 quickTuningGen.py --input-dir tunedData --op conv --arch gfx90a --update --no-splitk

	help='Removing the Split-K factor from the generated list')
	help='Remove the Split-K factor from the generated list')

		// Find the lexicographically closest relative
		static std::string findFallback(const std::string &target) {

Uh oh!

Address outstanding code review comments #1994

Are you sure you want to change the base?

Address outstanding code review comments #1994

Conversation

mirza-halilcevic commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

umangyadav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mirza-halilcevic Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhernandez0 Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

umangyadav commented Oct 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mirza-halilcevic commented Sep 19, 2025 •

edited

Loading

mirza-halilcevic Oct 25, 2025 •

edited

Loading

dhernandez0 Oct 21, 2025 •

edited

Loading