Skip to content

DotGeneralReshape: keep the dot's own result element type - #2986

Open
AsafManela wants to merge 1 commit into
EnzymeAD:mainfrom
AsafManela:fix-dotgeneralreshape-element-type
Open

DotGeneralReshape: keep the dot's own result element type#2986
AsafManela wants to merge 1 commit into
EnzymeAD:mainfrom
AsafManela:fix-dotgeneralreshape-element-type

Conversation

@AsafManela

Copy link
Copy Markdown

Fixes #2984.

DotGeneralReshape hoists a dim-inserting reshape from a dot_general's operand to its
result, but built the new dot with the operand element type. For a dot whose
accumulation type differs from its inputs -- the bf16 x bf16 -> f32 dots JAX emits with
preferred_element_type=f32 -- that gives a bf16 dot followed by a bf16 -> f32 reshape,
and the module stops verifying:

error: 'stablehlo.reshape' op requires compatible element types for all operands and results
  note: see current operation: %1 = "stablehlo.reshape"(%0) : (tensor<4x16xbf16>) -> tensor<1x4x16xf32>

Take the element type from the op being replaced instead. Dots whose result and operand
types agree are unaffected, which is why this only ever showed up on mixed-precision graphs.

Lit test: test/lit_tests/dotgeneralreshape_mixed_element_types.mlir.

Testing

The C++ is not built or run here -- no bazel in this environment (the reproducer and the
diagnosis come from the shipped Reactant_jll v0.0.405, where the pattern is the same
compiled code). The lit test is written against the current sources but has not been
executed; please let CI have it, and tell me if the CHECK lines need adjusting.

🤖 Generated with Claude Code

https://claude.ai/code/session_012k6KZhhCNSCvZFKzErHaHH

`DotGeneralReshape` hoists a dim-inserting reshape from a dot_general's
operand to its result, but built the new dot with the *operand* element type.
For a dot whose accumulation type differs from its inputs -- e.g. the
`bf16 x bf16 -> f32` dots JAX emits with `preferred_element_type=f32` -- that
produces a bf16 dot followed by a `bf16 -> f32` reshape, and the module no
longer verifies:

    error: 'stablehlo.reshape' op requires compatible element types for all
    operands and results
    note: see current operation:
      %1 = "stablehlo.reshape"(%0) : (tensor<4x16xbf16>) -> tensor<1x4x16xf32>

Take the element type from the op being replaced instead. Same-type dots are
unaffected.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@AsafManela

Copy link
Copy Markdown
Author

Update: this is now built and verified, so the "not built or run here" caveat in the
description no longer applies.

I compiled the patch into a JLL and compared it against an otherwise identical build from
the same pinned Enzyme-JAX commit, differing only by this change (confirmed by inspecting
each binary rather than trusting the build labels -- my first attempt at this comparison
was wrong because both bundles turned out to be patched).

Running the reproducer from the issue through dot_general_reshape:

without this patch:  error: 'stablehlo.reshape' op requires compatible element types for all operands and results
with this patch:     OK -- the dot keeps its f32 result and the trailing reshape is element-type preserving

A seeded fp32/bf16 smoke test gives bit-identical numerical results on both builds, so the
change is behaviour-preserving outside the failing case.

The lit test in this PR has still not been executed -- I verified through the compiled
pass rather than through enzymexlamlir-opt -- so please let CI have it and tell me if
the CHECK lines need adjusting.

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.

DotGeneralReshape emits invalid IR for dots whose result element type differs from their operands' (bf16 x bf16 -> f32)

1 participant