Skip to content

[ET-VK][ops] Add bitwise_or / logical_or operators#20382

Open
SS-JIA wants to merge 5 commits into
gh/SS-JIA/561/basefrom
gh/SS-JIA/561/head
Open

[ET-VK][ops] Add bitwise_or / logical_or operators#20382
SS-JIA wants to merge 5 commits into
gh/SS-JIA/561/basefrom
gh/SS-JIA/561/head

Conversation

@SS-JIA

@SS-JIA SS-JIA commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Adds Vulkan support for aten.bitwise_or.Tensor and aten.logical_or.default, mirroring the existing bitwise_and / logical_and implementation. This is the first of two ops needed to collapse the Llama4-mini TISO en_US backbone export to a single Vulkan partition: the discrete-speech mask OR-s several bool tensors via bitwise_or, which previously had no Vulkan implementation and forced a CPU fallback that split the delegated graph.

Implementation mirrors bitwise_and: a X | Y uint8 shader variant in binary_op_buffer.yaml / binary_op_texture.yaml, a DEFINE_BINARY_OP_FN(bitwise_or) dispatch with VK_REGISTER_OP for both aten.bitwise_or.Tensor and aten.logical_or.default in BinaryOp.cpp, and register_bitwise_or / register_logical_or OpFeatures (bool inputs) in op_registry.py.

This change was authored with Claude.

Differential Revision: D108457794

[ghstack-poisoned]
@pytorch-bot

pytorch-bot Bot commented Jun 18, 2026

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 1 New Failure

As of commit 6dc4f6b with merge base 1227757 (image):

NEW FAILURE - The following job has failed:

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

@linux-foundation-easycla

linux-foundation-easycla Bot commented Jun 18, 2026

Copy link
Copy Markdown

CLA Missing ID

@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 Jun 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.

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@metascroy

Copy link
Copy Markdown
Contributor

@claude review this code

@claude

claude Bot commented Jun 26, 2026

Copy link
Copy Markdown

Claude finished @metascroy's task in 1m 9s —— View job


Code Review: bitwise_or / logical_or

  • Read PR diff
  • Analyze shader/registry/test changes
  • Post review

Reviewed against origin/gh/SS-JIA/561/base. This is a clean, well-scoped change that faithfully mirrors the existing bitwise_and path. LGTM — no blocking issues.

Correctness ✅

  • BinaryOp.cpp: DEFINE_BINARY_OP_FN(bitwise_or) + the two VK_REGISTER_OP lines for aten.bitwise_or.Tensor and aten.logical_or.default are correct. Routing logical_or to the same uint8 X | Y shader is valid because the registry constrains inputs to BOOL_T (0/1), so bitwise-or and logical-or are equivalent on those values — same reasoning the existing logical_and → bitwise_and mapping relies on.
  • Shaders (binary_op_buffer.yaml / binary_op_texture.yaml): the X | Y uint8 variants match the bitwise_and pattern exactly. Kernel name (binary_bitwise_or_{buffer,texture3d}) lines up with what add_binary_op_node constructs at BinaryOp.cpp:76-80.

Nice improvements 👍

  • op_registry.py: consolidating five separate register_* functions into a single register_bool_binary_ops with a list of edge ops is a good simplification (net −12 lines) and keeps bitwise_not correctly grouped despite being unary, since it shares the same OpFeatures.
  • cases.py: dropping test_suite.data_gen = "make_seq_tensor" is a genuine test-quality fix, not just cosmetic. For at::kBool, make_seq_tensor fills values 1, 2, 3, … (all nonzero → all true), so OR/AND tests were effectively true | true everywhere. Reverting to the default make_rand_tensor (rand() > 0.5, see gen_correctness_base.py:315-316) exercises a real mix of true/false. The two added broadcasting shapes ((1, M2), (M1, M2)) and ((XS, 1, S1, 1), (1, S, 1, S2)) also broaden coverage — and since the suite is now shared, bitwise_and benefits from both changes too.

Minor observations (non-blocking)

  • The shared test suite means aten.logical_and.default is not in the @register_test_suite([...]) list, even though it's registered as an op. It was already untested before this PR, so no regression — just noting it for completeness if you want symmetric coverage with logical_or.
  • The CI failure (unittest-nxp-neutron) is in the NXP/Neutron backend and unrelated to these Vulkan changes.

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. meta-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants