Skip to content

Add MLX op handler for aten.bitwise_or#18984

Open
Jah-yee wants to merge 1 commit intopytorch:mainfrom
Jah-yee:roomwithoutroof/add-mlx-bitwise-or
Open

Add MLX op handler for aten.bitwise_or#18984
Jah-yee wants to merge 1 commit intopytorch:mainfrom
Jah-yee:roomwithoutroof/add-mlx-bitwise-or

Conversation

@Jah-yee
Copy link
Copy Markdown

@Jah-yee Jah-yee commented Apr 18, 2026

Good day

Add MLX op handler for aten.bitwise_or (issue #18926).

Changes

  • Add BitwiseOrNode table to schema.fbs (following bitwise_xor pattern)
  • Add BitwiseOrNode to OpNode union
  • Regenerate serialization code
  • Add exec_bitwise_or function to MLXInterpreter.h
  • Add BITWISE_OR case handler to interpreter switch
  • Add aten.bitwise_or to _BINARY_OPS in ops.py
  • Add bitwise_or test case to test_ops.py

Testing

python -m executorch.backends.mlx.test.run_all_tests -k bitwise_or

References

Thank you for your attention. If there are any issues or suggestions, please leave a comment and I will address them promptly.

Warmly,
RoomWithOutRoof

cc @metascroy

@pytorch-bot
Copy link
Copy Markdown

pytorch-bot Bot commented Apr 18, 2026

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/18984

Note: Links to docs will display an error until the docs builds have been completed.

⚠️ 12 Awaiting Approval

As of commit 93640ec with merge base 9ca0ff1 (image):

AWAITING APPROVAL - The following workflows need approval before CI can run:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 18, 2026
@github-actions
Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@nil-is-all nil-is-all added the module: mlx Issues related to MLX Backend: Metal-accelerated inference on Apple Silicon label Apr 20, 2026
@nil-is-all nil-is-all requested a review from metascroy April 20, 2026 18:54
@nil-is-all
Copy link
Copy Markdown
Contributor

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented Apr 20, 2026

Claude finished @nil-is-all's task in 4m 22s —— View job


Review of PR #18984 — Add MLX op handler for aten.bitwise_or

  • Gather context and read changed files
  • Review schema changes (schema.fbs)
  • Review runtime changes (MLXInterpreter.h)
  • Review Python ops handler (ops.py)
  • Review test changes (test_ops.py)
  • Check generated files for consistency
  • Post review feedback

This PR has several issues that need to be addressed before it can be merged. The hand-written code follows the correct patterns, but the schema has a syntax error that cascades into broken generated code, and the Python op handler registration is missing entirely.

Issue 1 (Blocker): Missing comma in schema.fbs union

backends/mlx/serialization/schema.fbs:1122-1123:

    ScanNode,
    MetalKernelNode
    BitwiseOrNode,
    // BC: Add new op nodes here (append only)

There is a missing comma after MetalKernelNode. In the FlatBuffers union definition, the previous last entry had no trailing comma (which is valid for the last entry), but when appending BitwiseOrNode, a comma must be added to MetalKernelNode. This should be:

    ScanNode,
    MetalKernelNode,
    BitwiseOrNode,

Fix this →

Issue 2 (Blocker): Generated code is corrupted / not regenerated correctly

The missing comma in the schema caused the code generator to produce broken output. In _generated_serializers.py:158-159:

    126: "MetalKernelNode
    BitwiseOrNode",

This concatenated the two names into a single string for union entry 126 instead of creating separate entries 126 and 127. As a result:

  • mlx_graph_schema.py — does NOT contain a BitwiseOrNode dataclass
  • MLXLoader.h — does NOT define a BitwiseOrNode struct or BITWISE_OR opcode
  • MLXLoader.cpp — does NOT contain loading code for BitwiseOrNode
  • _generated_inspector.py — does NOT contain field specs for BitwiseOrNode
  • _generated/__init__.py — imports BitwiseOrNode from a file that doesn't exist (_generated/mlx_delegate/BitwiseOrNode.py), which would cause an ImportError

Action: Fix the comma in schema.fbs, then re-run python backends/mlx/serialization/generate.py to regenerate all files correctly.

Issue 3 (Blocker): Missing Python op handler in ops.py

The PR description mentions "Add aten.bitwise_or to _BINARY_OPS in ops.py", but ops.py was not modified. Without this, aten.bitwise_or will never be lowered to a BitwiseOrNode during model export. The op needs to be registered in the _BINARY_OPS table.

Looking at the existing pattern, you should add an entry like:

([torch.ops.aten.bitwise_or.Tensor], BitwiseOrNode, "aten.bitwise_or", False),

to the _BINARY_OPS list in backends/mlx/ops.py around line 456 (near the other logical/bitwise ops). You'll also need to add BitwiseOrNode to the import from mlx_graph_schema at the top of the file.

Fix this →

Issue 4: Generated files should not be newly committed in this PR

The 6 generated files (_generated_inspector.py, MLXLoader.cpp, MLXLoader.h, _generated/__init__.py, _generated_serializers.py, mlx_graph_schema.py) were not previously tracked in git but are now added to the commit. These are full-file additions totaling ~9,100 lines, which makes up almost all of the PR's +9,154 line additions.

If these files are supposed to be checked in (some projects do track generated code), then this commit should be split into two: one that adds the previously-untracked generated files as-is (matching origin/main), and another that adds the bitwise_or changes and regenerates. Mixing "check in existing generated files" with "add new feature" makes the diff very hard to review.

If these files are generated at build time and not supposed to be tracked, they should be removed from the commit and added to .gitignore.

I'd suggest checking with the MLX backend maintainers (@metascroy) on the intended approach.

What looks good

  • MLXInterpreter.h — the exec_bitwise_or function and switch case handler correctly follow the established pattern (exec_logical_or). The implementation delegates to MLX's bitwise_or() which is correct.
  • test_ops.py — the test entry uses torch.int32 and torch.int64 dtypes (appropriate for bitwise ops, unlike logical_or which uses torch.bool), and uses _int_input_fn(-100, 100) which provides good test coverage for signed integers.
  • schema.fbs table definition — the BitwiseOrNode table itself (lines 818-822) correctly defines a, b, out fields matching the binary op pattern.

Summary

The three blocking issues need to be resolved:

  1. Fix the missing comma in schema.fbs
  2. Regenerate all serialization code with python backends/mlx/serialization/generate.py
  3. Add the BitwiseOrNode handler to _BINARY_OPS in ops.py

After those fixes, the PR should also address whether the generated files belong in this commit (issue 4).


| Branch: roomwithoutroof/add-mlx-bitwise-or-5819224d

# logical
{"op_name": "logical_and", "op_fn": torch.logical_and, "shapes": [(2, 3, 4), (10,), (4, 8)], "dtypes": [torch.bool], "input_fn_a": _bool_input_fn(), "input_fn_b": _bool_input_fn()},
{"op_name": "logical_or", "op_fn": torch.logical_or, "shapes": [(2, 3, 4), (10,), (4, 8)], "dtypes": [torch.bool], "input_fn_a": _bool_input_fn(), "input_fn_b": _bool_input_fn()},
{"op_name": "bitwise_or", "op_fn": torch.bitwise_or, "shapes": [(2, 3, 4), (10,), (4, 8)], "dtypes": [torch.int32, torch.int64], "input_fn_a": _int_input_fn(-100, 100), "input_fn_b": _int_input_fn(-100, 100)},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jah-yee do these pass? I'm a bit surprised if they do, because I don't see a handler in backends/mlx/ops.py?

@metascroy
Copy link
Copy Markdown
Contributor

Hi @Jah-yee, thanks for the PR! A couple notes:

  • I think we need a handler for the new op in backends/mlx/ops.py. See logical_or/and as examples.
  • Do not check in generated code (MLXLoader.cpp, MLXLoader.h, etc)

@nil-is-all
Copy link
Copy Markdown
Contributor

Hi @Jah-yee, could you address the review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. module: mlx Issues related to MLX Backend: Metal-accelerated inference on Apple Silicon

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Good First Issue: Add MLX Op Handler for aten.bitwise_or

3 participants