Add support for migraphx.max

[pfultz2]

Motivation

The migraphx.max is needed to support leaky relu efficiently.

Technical Details

Test Plan

Test Result

Submission Checklist

• Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

[rocmlir-pr-reviewer]

Verdict: APPROVE -- submitted as COMMENT (automated reviews are advisory)  ·  Findings: 0 (0 Critical, 0 Major, 0 Minor)

---

Scope

Adds a new migraphx.max elementwise binary op to support efficient leaky relu. The change registers the op in MIGraphX.td (with Commutative), wires up lowerings to linalg.max and tosa.maximum, and adds Lit conversion tests.

Findings

No blocking issues found.

Notes

• The op definition, both conversion registrations, and the tests all follow the established elementwise-binary-op patterns in each file exactly. Commutative is appropriate for max.
• Dtype coverage on the TOSA path is good (f32/f16/i32 tests). The Linalg test covers only f32, but that is consistent with the convention used by the other elementwise ops in mixr-to-linalg-ops.mlir, so no gap is introduced.
• Consider (optional, out of scope) adding a negative/verifier Lit test in a follow-up if migraphx.max should reject mismatched element types, though AllElementTypesMatch on the base class already provides this and mirrors sibling ops that also lack such a test.

CI status

No non-self CI checks are in the fail/cancel buckets. The passing premerge checks (C/C++, Python format/lint) are green; the remaining Jenkins/Build/MIGraphX checks are still pending. The auto-review pipeline's own review check being in-progress is expected and not a CI failure.

[Copilot]

Pull request overview

This PR adds a new MIGraphX dialect elementwise binary op (migraphx.max) and wires it into existing MIGraphX lowering pipelines so it can be converted to both TOSA and Linalg IR.

Changes:

• Introduces MIGraphX_MaxOp (migraphx.max) in the MIGraphX dialect TableGen.
• Adds MIGraphX→TOSA lowering via tosa.maximum.
• Adds MIGraphX→Linalg lowering via linalg.max and extends conversion tests accordingly.

Reviewed changes

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

Show a summary per file

File	Description
mlir/include/mlir/Dialect/MIGraphX/IR/MIGraphX.td	Adds MIGraphX_MaxOp as an elementwise binary op (commutative).
mlir/lib/Conversion/MIGraphXToTosa/MIGraphXToTosa.cpp	Registers migraphx.max lowering to tosa.maximum.
mlir/lib/Conversion/MIGraphXToLinalg/MIGraphXToLinalg.cpp	Registers migraphx.max lowering to linalg.max.
mlir/test/Conversion/MIGraphXToTosa/mixr-to-tosa-ops.mlir	Adds FileCheck coverage for max lowering (f32/f16/i32).
mlir/test/Conversion/MIGraphXToLinalg/mixr-to-linalg-ops.mlir	Adds FileCheck coverage for max lowering to linalg.max.

---

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

[codecov]

Codecov Report

❌ Patch coverage is 89.01099% with 10 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...irCustomTosaToLinalg/RocmlirCustomTosaToLinalg.cpp	53.85%	1 Missing and 5 partials ⚠️
mlir/lib/Dialect/MIGraphX/IR/MIGraphX.cpp	90.00%	0 Missing and 2 partials ⚠️
...b/Conversion/MIGraphXToLinalg/MIGraphXToLinalg.cpp	96.88%	0 Missing and 1 partial ⚠️
...r/lib/Conversion/MIGraphXToTosa/MIGraphXToTosa.cpp	96.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2429      +/-   ##
===========================================
+ Coverage    82.57%   83.66%   +1.09%     
===========================================
  Files          120      120              
  Lines        42852    43017     +165     
  Branches      7110     7153      +43     
===========================================
+ Hits         35381    35988     +607     
+ Misses        4815     4482     -333     
+ Partials      2656     2547     -109     

Flag	Coverage Δ
gfx120x	83.48% <89.01%> (+0.96%)	⬆️
gfx950	83.45% <89.01%> (+1.10%)	⬆️
mfma	83.27% <89.01%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...r/include/mlir/Dialect/Rock/IR/RockTosaCustomOps.h	100.00% <100.00%> (ø)
...b/Conversion/MIGraphXToLinalg/MIGraphXToLinalg.cpp	88.98% <96.88%> (+0.17%)	⬆️
...r/lib/Conversion/MIGraphXToTosa/MIGraphXToTosa.cpp	93.08% <96.00%> (+0.16%)	⬆️
mlir/lib/Dialect/MIGraphX/IR/MIGraphX.cpp	84.16% <90.00%> (+5.02%)	⬆️
...irCustomTosaToLinalg/RocmlirCustomTosaToLinalg.cpp	78.75% <53.85%> (-1.81%)	⬇️

... and 36 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[pabloantoniom]

What's the plan for backporting to rocmlirTriton?

[umangyadav]

This PR has some problem related to propagating signedness.

migraphx.max accepts every integer type, including ui8/ui32, but both paths eventually use signed maximum:

[MIGraphX→Linalg] creates linalg.max, whose integer implementation is arith.maxsi.
MIGraphX→TOSA also loses integer signedness, and TOSA→Linalg lowers integer tosa.maximum to arith.maxsi.
For example, max(ui8(255), ui8(1)) becomes signed max(-1, 1) and returns 1.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

[rocmlir-pr-reviewer]

This asserts every operand equals the 2x4 result type, but the new broadcast test max_ui32_broadcast in mlir/test/Conversion/MIGraphXToTosa/migraphx-max-semantics.mlir:39 expects the resulting linalg.generic to have ins(... : tensor<2x4xi32>, tensor<1x4xi32>). These two expectations cannot both hold: if a broadcasted (rank-matched, unequal) operand actually reaches this converter, this assert fires in an assertions-enabled build; and the unsigned custom-op is then lowered in RocmlirCustomTosaToLinalg.cpp with identity indexing maps for all inputs, which requires every linalg.generic input to have the output shape — a 1x4 input against a 2x4 output would fail linalg.generic verification. Conversely, if operands are always equalized to 2x4 (as the Linalg ElementwiseConverter requires), then the 1x4 FileCheck expectation won't match. Please confirm the unsigned-broadcast path builds valid IR (ideally via an assertions build) and reconcile the assert with the test; if broadcasting can reach the custom op, the identity-map lowering needs broadcast-aware indexing maps rather than pure identity maps.

[umangyadav]

This turns out to be a false positive — the unsigned-broadcast path builds valid IR, and the assert never sees unequal operand types. I verified against an assertions-enabled build (LLVM_ENABLE_ASSERTIONS=ON).

The key point is that a broadcasted migraphx.shaped<2x4xui32, 0x1> operand is materialized into a full 2x4 tensor before MaxConverter runs. Lowering func.func(migraphx-to-tosa) on the broadcast case gives:

%5 = tosa.mul %3, %2, %4 : (tensor<2x4xi32>, tensor<1x4xi32>, tensor<1xi8>) -> tensor<2x4xi32>
%8 = tosa.custom %7, %5 {... operator_name = "unsigned_max"} : (tensor<2x4xi32>, tensor<2x4xi32>) -> tensor<2x4xi32>

So both operands reaching the custom op are 2x4, assert(value.getType() == resultType) holds, and the subsequent identity-map lowering in RocmlirCustomTosaToLinalg.cpp is valid. After the full pipeline the two generics are:

#map1 = affine_map<(d0, d1) -> (0, d1)>
%6  = linalg.generic {indexing_maps = [#map, #map1, #map], ...} ins(%3, %transposed : tensor<2x4xi32>, tensor<1x4xi32>) ... arith.muli   // broadcast materialization
%10 = linalg.generic {indexing_maps = [#map, #map,  #map], ...} ins(%8, %6         : tensor<2x4xi32>, tensor<2x4xi32>) ... arith.maxui  // the max

The tensor<1x4xi32> the comment referenced belongs to the mul-by-ones broadcast generic (which correctly uses the broadcast map (0, d1)), not to the arith.maxui generic. The two FileCheck lines refer to two different linalg.generic ops, so they don't conflict. Both migraphx-max-semantics.mlir lit tests pass.

The confusion is fair, though — the check was written ambiguously. It's now tightened to bind the broadcast result and anchor the shape check to the max generic (ins(..., %[[BROADCAST]] : tensor<2x4xi32>, tensor<2x4xi32>)), and I added an explanatory comment in cf7f698 documenting that broadcast expansion precedes the max. Resolving as addressed.

[rocmlir-pr-reviewer]

Resolved -- addressed in this revision.

[rocmlir-pr-reviewer]

Verdict: COMMENT  ·  Findings: 1 (0 Critical, 1 Major, 0 Minor)

---

Scope

Adds migraphx.max (elementwise IEEE-754 maximum) to the MIGraphX dialect and lowers it through both the TOSA and Linalg boundaries, including an unsigned_max custom-op path for unsigned integers. Touches the op definition (.td), verifier, both converters, the custom-TOSA→Linalg lowering, and adds dialect, conversion, and e2e tests.

Findings

• mlir/lib/Conversion/MIGraphXToTosa/MIGraphXToTosa.cpp:1119 — the equal-shape assert on the new MaxConverter appears to be in tension with the new broadcast semantics test; please confirm the unsigned-broadcast path produces valid IR (details inline).

Notes

• Op design is solid: hasVerifier = 1 + verify() present, and the verifier correctly relies on the AllShapesMatch/AllElementTypesMatch traits (checked before verify()) so testing only inA for element type / static shape is fine, while strides are checked on all three operands.
• Semantics look correct: floats use tosa.maximum (PROPAGATE) / linalg.max→arith.maximumf (IEEE maximum, NaN-propagating, +0 > -0); signed/signless ints use signed max; unsigned ints use arith.maxui. Matches the documented description.
• The UnsignedCastLoweringPattern→UnsignedOpLoweringPattern rename and updated notifyMatchFailure message are consistent with the broadened responsibility.
• Test coverage is thorough: negative cases in invalid.mlir (mismatched shapes, unsupported/complex element type, dynamic shape, dynamic stride, i1), roundtrip in ops.mlir, conversion checks, an e2e template registered in tests.toml, and a PR-e2e IEEE test exercising NaN/±0/inf.

CI status

No non-self checks report fail/cancel; all functional checks (C/C++ premerge, Python, review) are still IN_PROGRESS at review time, so lit/e2e results were not available to confirm the broadcast case above.

[rocmlir-pr-reviewer]

Verdict: APPROVE -- submitted as COMMENT (automated reviews are advisory)  ·  New findings: 0 (0 Critical, 0 Major, 0 Minor)

---

Scope

Adds a new migraphx.max op (elementwise IEEE-754 maximum) with a verifier, plus TOSA and Linalg lowering paths. Floating-point and signed/signless integers lower to tosa.maximum (PROPAGATE) / linalg.max; unsigned integers route through a rocmlir/unsigned_max custom op that lowers to arith.maxui. Touches the dialect def, both conversion passes, the custom-tosa-to-linalg pass, and adds unit, conversion, custom-op, and E2E tests.

Findings

No blocking issues found.

Notes

• Semantics are well documented (op description) and enforced: the verifier rejects dynamic shapes/strides, non int/float element types, and i1, each with a matching negative lit test. AllShapesMatch + the base AllElementTypesMatch cover the operand/result shape and element-type constraints, so the verifier's single-operand checks are sufficient.
• IEEE maximum (NaN propagation, +0 > -0) is exercised across both lowerings via migraphx-max-semantics.mlir (TOSA and Linalg) and a dedicated runtime E2E mixr-max-ieee.e2e.mlir. The unsigned path has good lit coverage (rocmlir-custom-tosa-to-linalg.mlir, semantics test).
• Minor (non-blocking): the fusion E2E template mixr-gemm-max only expands over f32/f16/bf16, so the integer / unsigned arith.maxui custom-op path has lit coverage but no runtime E2E. Worth considering in a follow-up, not required here.

CI status

No non-self CI failures at the reviewed SHA: C/C++ premerge, Python format/lint, and performance-script checks pass; Jenkins/Build-and-Test are still pending. The auto-review pipeline's own review check being in-progress is expected.

Reconciliation

The prior review's concern about the assert in MaxConverter conflicting with the max_ui32_broadcast expectation was rebutted by a maintainer (broadcast is materialized before the max op runs, verified on an assertions build) and the test was tightened to bind the broadcast result. Fresh review agrees it is not a real issue; resolving that thread.