-
Notifications
You must be signed in to change notification settings - Fork 59
[AIROCMLIR-426] Translate rocMLIR conv layouts to MIOpen or skip #2422
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
Changes from 3 commits
c4e0d9e
97df95e
62f3e33
7e50442
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,6 +98,61 @@ def inverse_filter_layouts(filter_layout): | |
| return "".join(map[char] for char in filter_layout) | ||
|
|
||
|
|
||
| # MIOpenDriver only understands the NCHW / NHWC memory layouts, whereas rocMLIR | ||
| # configs use richer names such as GNC01 / NGC01 that additionally encode the group | ||
| # dimension (G) and use 0/1 for the spatial dims. A config can therefore only be | ||
| # benchmarked against MIOpen when its layouts map *exactly* onto NCHW/NHWC once the | ||
| # group dimension is dropped (MIOpen conveys the group count separately via -g) and | ||
| # the spatial dims are renamed 0->H, 1->W. Layouts with any other ordering have no | ||
| # faithful MIOpen equivalent and are skipped instead of being benchmarked against a | ||
| # different layout (which would be an unfair comparison). | ||
| MIOPEN_CONV_LAYOUTS = {'NCHW', 'NHWC'} | ||
|
|
||
|
|
||
| def rocmlir_layout_to_miopen(layout): | ||
| """Map a rocMLIR conv layout name onto a MIOpenDriver layout, or None. | ||
|
|
||
| The group dimension ``G`` is dropped (MIOpen passes the group count through the | ||
| separate ``-g`` flag) and the spatial dims are renamed (``0`` -> ``H``, ``1`` -> | ||
| ``W``). MIOpen spells every tensor's layout generically as NCHW/NHWC, so the | ||
| output tensor's channel letter ``K`` is treated like ``C``. | ||
|
|
||
| Returns ``"NCHW"`` or ``"NHWC"`` when the layout is exactly one of those orderings, | ||
| otherwise ``None`` -- meaning the config is not MIOpen-representable and should be | ||
| skipped to keep the comparison fair. | ||
| """ | ||
| normalized = layout.replace('0', 'H').replace('1', 'W').replace('G', '').replace('K', 'C') | ||
| if normalized in MIOPEN_CONV_LAYOUTS: | ||
| return normalized | ||
| return None | ||
|
|
||
|
|
||
| def conv_commandline_to_miopen_layouts(commandline): | ||
| """Translate rocMLIR conv layout args (-f/-I/-O) into MIOpen layout names. | ||
|
|
||
| Returns a new commandline list with the layout values replaced by their MIOpen | ||
| equivalents, or ``None`` when the configuration has no faithful MIOpen | ||
| representation -- either because a layout uses an ordering MIOpen cannot express, | ||
| or because the filter/input/output tensors do not share a single NCHW/NHWC layout. | ||
| Callers should skip the MIOpen benchmark in the ``None`` case rather than run an | ||
| unfair comparison. | ||
| """ | ||
| result = list(commandline) | ||
| layout_flags = {'-f', '-I', '-O'} | ||
| seen_layouts = set() | ||
| for i in range(len(result) - 1): | ||
| if result[i] in layout_flags: | ||
| miopen_layout = rocmlir_layout_to_miopen(result[i + 1]) | ||
| if miopen_layout is None: | ||
| return None | ||
| result[i + 1] = miopen_layout | ||
| seen_layouts.add(miopen_layout) | ||
| # MIOpen expects a single, consistent layout across filter, input and output. | ||
| if len(seen_layouts) > 1: | ||
| return None | ||
|
Comment on lines
+151
to
+152
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this really true ? can you double check ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I double-checked directly with MIOpenDriver on a gfx942 GPU, same conv, only changing the layouts: Same config runs fine for all-NCHW and all-NHWC, but fails the moment the layouts are mixed, MIOpen has no solver for mixed filter/input/output layouts. That's why I skip those configs so if we translated and ran them, the fail-on-error path would turn the nightly red |
||
| return result | ||
|
bogdan-petkovic marked this conversation as resolved.
|
||
|
|
||
|
|
||
| @dataclass | ||
| class MLIRPaths: | ||
| rocmlir_gen_path: str | ||
|
|
@@ -828,20 +883,35 @@ def benchmark_external(cls, commandline, paths: Paths, arch, num_cu, num_chiplet | |
| if config.datatype not in cls.MIOPEN_SUPPORTED_DTYPES: | ||
| print(f"Skipping MIOpen benchmark for unsupported datatype: {config.datatype}") | ||
| return config.table_entry(np.nan) | ||
| miopen_driver_cmd = [MIOPENDRIVER, *commandline, '-V', '0', '-t', '1'] | ||
| print("Running MIOpen Benchmark: ", ' '.join(commandline)) | ||
| # rocMLIR configs use layout names (e.g. GNC01) that MIOpenDriver rejects. | ||
| # Translate them to NCHW/NHWC; skip configs that have no faithful MIOpen | ||
| # equivalent instead of forcing an unfair comparison. | ||
| miopen_commandline = conv_commandline_to_miopen_layouts(commandline) | ||
| if miopen_commandline is None: | ||
| print("Skipping MIOpen benchmark: conv layout has no equivalent MIOpen " | ||
| f"NCHW/NHWC representation: {' '.join(commandline)}") | ||
| return config.table_entry(np.nan) | ||
| miopen_driver_cmd = [MIOPENDRIVER, *miopen_commandline, '-V', '0', '-t', '1'] | ||
| print("Running MIOpen Benchmark: ", ' '.join(miopen_driver_cmd)) | ||
| # invoke MIOpenDriver. | ||
| outs, noerr = run_pipeline([miopen_driver_cmd]) | ||
| nanoseconds = np.nan | ||
| if noerr: | ||
| # convert bytes to str | ||
| outs = outs.decode('utf-8') | ||
| # Extract Elapsed time in ms from the output of MIOpenDriver | ||
| # Use regular expression to match the contents between | ||
| # "Elasped: " (note the space at the end) and "ms" | ||
| elapsed_time_in_ms = ELAPSED_TIME_RE.search(outs).group(1) | ||
| nanoseconds = float(elapsed_time_in_ms) * 1.0e6 | ||
|
|
||
| if not noerr: | ||
| # run_pipeline already prints MIOpenDriver's stderr. A genuine MIOpen failure | ||
| # must fail CI instead of silently yielding NaN. | ||
| raise RuntimeError("MIOpen benchmark failed (see the MIOpenDriver error above); " | ||
| "CI must fail on MIOpen errors.\n" | ||
| f"Failing command: {' '.join(miopen_driver_cmd)}") | ||
|
bogdan-petkovic marked this conversation as resolved.
|
||
| # convert bytes to str | ||
| outs = outs.decode('utf-8') | ||
| # Extract Elapsed time in ms from the output of MIOpenDriver. Match the text | ||
| # between "Elapsed: " (note the trailing space) and "ms". | ||
| match = ELAPSED_TIME_RE.search(outs) | ||
| if not match: | ||
| raise RuntimeError("Failed to parse elapsed time from MIOpenDriver output.\n" | ||
| f"Failing command: {' '.join(miopen_driver_cmd)}\n" | ||
| f"Output:\n{outs}") | ||
| elapsed_time_in_ms = match.group(1) | ||
| nanoseconds = float(elapsed_time_in_ms) * 1.0e6 | ||
| return config.table_entry(nanoseconds) | ||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not make these helper functions members of
ConvConfigurationsimilar to howbenchmark_externalis already setup?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put these right next to the other layout helpers on purpose. input_layouts, filter_layouts and the inverse ones are all plain module level functions too, and they're actually shared between ConvConfiguration and ConvGemmConfiguration, which is why they don't live inside a single class. benchmark_external is a classmethod because it genuinely needs cls and things like table_entry and from_command_line. These two new ones are just pure string helpers with no class state, so making them staticmethods would mostly add a class prefix while making them inconsistent with the helpers sitting right above them. I'm happy to move them into ConvConfiguration if you'd prefer, I just wanted to keep it consistent with what's already there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay. I'm okay with leaving them next to the other layout helpers then.