migraphx-simplify: Code Review and Cleanup

You are improving the quality of the changed code, not hunting for bugs. Review it for reuse, simplification, efficiency, altitude, safety, and comment quality, then fix what you find. Do not look for correctness bugs — that is what /code-review is for.

Phase 0 — Gather the diff

Determine the set of changed code under review, then hand the same diff to every agent so they all see identical context.

The base must be the up-to-date remote integration branch — use origin/develop, not the local develop ref. The local develop is frequently stale (behind origin by several already-merged PRs), and feature branches often merge develop in repeatedly. Diffing against a stale local develop silently pulls already-merged upstream PRs into the diff, so you end up reviewing and editing files that aren't this branch's work at all.

1. Resolve the base ref. Prefer origin/develop; if it doesn't resolve, fall back in order to origin/main, origin/master, then a local develop / main / master — but never default to a local branch when a remote-tracking ref exists. Optionally git fetch origin develop first if the network is available and you suspect origin/develop itself is stale.

   git rev-parse --abbrev-ref HEAD
   git rev-parse --verify --quiet origin/develop   # confirm the base resolves

   Use the resolved ref as <base> below.
2. Produce the combined diff of committed-on-this-branch and uncommitted working-tree changes versus the merge base:

   git merge-base HEAD <base>
   git diff $(git merge-base HEAD <base>)

3. Sanity-check the scope before launching agents. List the changed files and confirm they look like this branch's work:

   git diff --stat $(git merge-base HEAD <base>) HEAD

   If the list contains files you don't expect — unrelated ops, whole subsystems, or infra you didn't touch — the base is almost certainly wrong. A tell-tale sign is that HEAD is a Merge branch 'develop' into <feature> commit (its develop-side parent HEAD^2 is usually the correct base). Confirm a suspect commit is upstream (and therefore out of scope) before trusting the diff:

   git merge-base --is-ancestor <commit-sha> <base>   # exit 0 => upstream, exclude it

   Re-resolve <base> (e.g. to HEAD^2) until the file list matches only the branch's own changes.
4. If the diff against the base is empty (e.g. you are already on the base branch), fall back to the working-tree changes alone:

   git diff HEAD
   git status --short

5. List the changed files and note which are GPU kernel headers (under src/targets/gpu/kernels/) versus general host C++ — the kernel-specific bullets below only apply to kernel files.

If there are no changes at all, stop and report that there is nothing to review.

Phase 1 — Review (6 cleanup agents in parallel)

Launch 6 independent review agents via the Agent tool, all in a single message so they run concurrently (use subagent_type: general-purpose; they need Grep/Read to inspect the surrounding codebase). Give each agent the full diff from Phase 0, the list of changed files, and the one review angle below that is its responsibility.

Tell every agent: assume all tools work — do not test them or make exploratory calls without a purpose. Each agent returns its findings as a list, where each finding has file, line, a one-line summary, and the concrete cost (what is duplicated, wasted, unsafe, misleading, or harder to maintain) plus the concrete fix (name the helper, the simpler form, or the safe abstraction to use instead). Report only findings inside the changed code; do not flag pre-existing issues.

1. Reuse

Flag new code that re-implements something the codebase or standard library already provides. Grep shared/utility modules and files adjacent to the change before concluding something is new, and name the existing helper to call instead.

• Flag any new function that duplicates existing functionality - Suggest the existing function to use instead.
• Flag any inline logic that could use an existing utility — hand-rolled string manipulation, manual path handling, custom environment checks, ad-hoc type guards, and similar patterns are common candidates.
• Copy-paste with slight variation: near-duplicate code blocks that should be unified with a shared abstraction
• Raw loops that should be STL <algorithm> or MIGraphX <migraphx/algorithm.hpp> algorithms (transform_if, transform_accumulate, group_by, group_unique, adjacent_for_each, etc.). std::for_each is not an acceptable substitute for a real algorithm, but a raw loop is the least preferred form: never rewrite a std::for_each into a raw for loop. If it is a side-effecting fold with no clean algorithm (e.g. folding hash_combine into a seed), leave it alone.
• A contiguous inserted before reshape — reshape already copies and handles non-standard input layouts, so the extra op is redundant.
• Pair/tuple member access (auto r = f(); r.first) where a structured binding auto [a, b] = f(); fits; when a C++17 lambda needs the binding, use an init-capture ([&, &x = x]) rather than avoiding the binding.
• Manually written lexicographical comparisons — use std::tie or std::lexicographical_compare instead.
• Manual offset / stride / index math with mod and division — use the migraphx::shape methods for offsets, strides, and indexing instead.
• Open-coded instruction traversal where a matcher from src/matcher.hpp would do.
• C acquire/release handling that should use MIGRAPHX_MANAGE_PTR.
• Duplicated literals or magic numbers where a named constant or existing enum already exists.
• A new helper that duplicates one already living elsewhere — name the existing function.
• Not a duplicate: a bespoke test stub operator (e.g. view_pass_op in test/gpu/hipgraphify.cpp) that a basic_ops stub could replace behaviorally. Check which branch of the predicate under test each one drives (no compute → not context-free, versus context-free + aliasing); if they differ, swapping them silently changes what the test covers.
• (kernel files) Hand-rolled utilities that already exist: migraphx::array, tensor_view, vec<T, N> for vector types, repeat_c for unrolling loops, uninitialized_buffer for shared LDS, the index class for thread/block indices, and local_stride / global_stride / local_wave_stride for strided loops.

2. Simplification

Flag unnecessary complexity the diff adds — code that could do the same job in a simpler, smaller, or flatter form. Name the simpler form that does the same job.

• Avoid redundant or unnecessary casts.
• Redundant or derivable state; intermediate variables that can be used directly.
• Copy-paste with slight variation that should be factored or parameterized.
• Deep nesting that an early return or restructuring would flatten.
• Dead code left behind by the change. Not dead code: a shared kernel utility (e.g. partial in kernels/functional.hpp) whose last caller this diff removed — the kernel functional library is kept as a toolkit, so only remove helpers the change itself introduced.
• Premature abstraction — a helper used only once.
• Classes masquerading as functions — a stateless struct with one public method instantiated as an object should be a free function. A static member function that happens to use no member state is not this: it is scoped to its class on purpose, so leave it where it is.
• Locally-called named lambdas (auto used_outside = [&](...) {...}; followed by a call) — a function in disguise with hidden captures. Convert to a static free function taking its captures as parameters, or inline a single use. Lambdas passed to an algorithm or callback are fine.
• Static helpers that each take 5+ parameters to marshal shared state after a function was split — replace with a (nested) struct holding that state in value members, with methods that add to it; keep context& and other external state as method parameters.
• Config / builder structs around what should be 1–2 function arguments; factory functions that only call a constructor.
• Multiple wrapper layers with no distinct responsibility per layer — collapse them.
• Future-proofing: single-value enums, one-alternative variant, always-present optional returns, hooks/callbacks with no second caller.
• Unnecessary arithmetic (+0, *1).
• Defensive checks at internal boundaries and speculative error handling for conditions that cannot occur given caller guarantees — remove them. (This is distinct from assertions that document assumptions, which belong to the safety pass.) An assert at an exported function's boundary that a later internal helper re-asserts is not a duplicate: each layer documents its own contract, so keep both.
• Backwards-compat shims that aren't needed: renamed _unused parameters, re-exports of removed types.
• Unrequested refactors bundled into the change — flag them for separation.
• A one-line layout predicate (e.g. is_nhwc(const shape&)) added to a widely included core header such as permutation.hpp — inline the stride check at each site with a comment instead; narrow GPU/conv helpers do not belong in core headers.
• (kernel files) ::value used to read an integral constant — it converts implicitly; pass the integral_constant through and capture it with auto. In tests, ::value is read on purpose to check the stored constant: keep it and add a cppcheck-suppress migraphx-AvoidNestedValue.
• (kernel files) Do not flag if constexpr special cases that look redundant with the generic path (e.g. arg_c for N == 0): kernel headers are JIT-compiled at runtime and those branches cut template instantiations. If one lacks a comment saying so, add the one-line comment rather than removing the branch.
• (kernel files) Do not merge per-kernel JIT raw-string source templates into a shared ${...} shell; the explicit per-kernel template is the preferred form.
• (kernel files) static_cast<T&&>(x) is a move, not a redundant cast — keep it and suppress migraphx-RedundantCast; do not introduce a move() helper.

3. Efficiency

Flag wasted work the diff introduces — computation, memory, or build cost that a cheaper form avoids without changing behavior. Name the cheaper alternative.

• Redundant computation or repeated I/O that could be hoisted or cached.
• Independent operations run sequentially that could be combined.
• Blocking or expensive work added to startup or a hot path.
• Copies where a reference or view suffices; unnecessary auto&&.
• Unused std::move and unused casts.
• Over-broad #includes where a forward declaration or a narrower header would do.
• A non-template helper defined inline in a header — it drags its includes into every consumer. Declare it in the header (with the target's export macro) and move the body to the .cpp; only templates and trivial accessors stay inline.

4. Altitude

Check that each change is implemented at the right depth, not as a fragile bandaid, and that it is written generically rather than tied to one concrete type. Special cases layered on shared infrastructure signal the fix isn't deep enough — prefer generalizing the underlying mechanism.

• Special cases added on top of shared infrastructure where generalizing the mechanism would be cleaner than another branch.
• Type-specific code that should be generic — write reusable, type-independent code using templates and STL algorithms rather than duplicating per type.
• Open-coded traversal where a matcher-based rewrite expresses the transform at the right level.
• Hand-rolled polymorphism via inheritance where MIGraphX's type erasure (implement the interface, no base class) fits.
• Leaky abstractions - exposing internal details that should be encapsulated, or breaking existing abstraction boundaries

5. Safety

Flag places where the diff gives up safety it could keep for free — unchecked assumptions, raw memory and pointer handling, and unsafe type or bit manipulation. Do not recommend adding defensive checks at internal boundaries or speculative error handling; assertions that document an invariant are different and are encouraged.

• Silent assumptions (a size matches, an index is in range, a pointer is non-null, a shape is packed/standard) — add an assert / MIGraphX assertion that documents and checks the invariant.
• Manual buffers or raw C arrays — use std::vector / std::array; in kernels use migraphx::array.
• Raw new / delete — use std::make_unique / std::make_shared; use MIGRAPHX_MANAGE_PTR for C-style acquire/release resources.
• Raw pointer arithmetic — use iterators, range-based access, or tensor_view; in kernels prefer tensor_view / iterators over raw pointers and the index class over raw threadIdx / blockIdx.
• Manual offset / index math that can run out of bounds — compute via migraphx::shape (host) or tensor_view / shape / index (kernels); slice a tensor_view / shape for tiling rather than hand-computing offsets, and use uninitialized_buffer for shared LDS.
• Unsafe casts — const_cast (especially to bypass API design), C-style casts, and reinterpret_cast that bypass the type system; fix by declaring the correct type at the source.
• Bit-twiddling and low-level bit manipulation written inline — encapsulate it behind a well-named utility function and prefer std::bitset.

6. Comments

Check that comments in the diff are correct, clear, and relevant, and remove comment slop. Quote the current comment and give the corrected / shortened replacement, or recommend deletion.

• Correct — the comment matches what the code actually does; flag stale comments left after the code changed and comments describing the wrong behavior.
• Clear — flag confusing, ambiguous, or misleading wording; rewrite to state the non-obvious why (a constraint, workaround, or invariant) in one to three lines. A rewrite must not grow the comment: drop parentheticals, cross-references, and enumerated consequences unless they are load-bearing.
• Relevant — flag comments that discuss unrelated components, files, or code not present in the change.
• Tombstones — flag // removed ... / "this used to ..." comments that describe history instead of the current code.
• Slop — delete redundant comments describing what obvious code does (// increment i, // return result).
• Banners — collapse multi-line docstrings / banner comments on trivial functions to one short line at most.

Phase 2 — Apply the fixes

Wait for all six agents to complete. Dedup findings that point at the same line or mechanism (the angles overlap by design — e.g. reuse and safety may both flag a raw pointer that should be a tensor_view). Fix each remaining finding directly in the working tree with Edit.

Skip any finding whose fix would:

• change intended behavior,
• require changes well outside the reviewed diff, or
• that you judge to be a false positive

— note the skip rather than arguing with it. Do not modify tests to match changed output; if a fix breaks a test, the fix is wrong.

Finish with a brief summary grouped by angle: what was fixed (with file:line) and what was skipped and why — or confirm the code was already clean.