Scan support - #16028

Merged
JacobSzwejbka merged 20 commits into
mainfrom
scan_support
Dec 11, 2025
Merged

Scan support#16028
JacobSzwejbka merged 20 commits into
mainfrom
scan_support

Conversation

@JacobSzwejbka

@JacobSzwejbkaJacobSzwejbka commented Dec 1, 2025

Copy link
Copy Markdown
Contributor

Add support for higher order ops scan. Its inefficient today because we are manually deep copying from output to input for every carry. We could do better by shallow swapping the pointers but Ill do that in a follow up if needed.

Test plan: Unit tests and internal verification against harder patterns

@pytorch-bot

pytorch-botBot commented Dec 1, 2025•
edited
Loading

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 1 New Failure, 1 Unrelated Failure

As of commit 90e55dd with merge base 9eaea4a (image):

NEW FAILURE - The following job has failed:

UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:

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

@meta-clameta-claBot 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 Dec 1, 2025
@github-actions

github-actionsBot commented Dec 1, 2025

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.

@meta-codesync

meta-codesyncBot commented Dec 1, 2025

Copy link
Copy Markdown
Contributor

@JacobSzwejbka has imported this pull request. If you are a Meta employee, you can view this in D88107948.

@JacobSzwejbkaJacobSzwejbka changed the title [WIP] Scan supportScan supportDec 9, 2025
larryliu0820
larryliu0820 reviewed Dec 9, 2025
View reviewed changes
Comment threadexir/emit/test/test_emit.py
op_table = program.execution_plan[0].operators
instructions = program.execution_plan[0].chains[0].instructions

# Collect all operator names in the program

@larryliu0820larryliu0820Dec 9, 2025

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.

honestly all the ops seem like implementation details and should not be tested

@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I was using it as a sort of a proxy that the general pattern was emitted. If you want we can just test the end 2 end behavior though.

@larryliu0820larryliu0820Dec 10, 2025

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.

Don't have a strong opinion, but you might have to maintain this test if there's a change to the exported graph in the future

@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The ops we are querying over are the ones /not/ in the original model definition but instead created by the emitter to maintain the semantics of scan

larryliu0820
larryliu0820 approved these changes Dec 10, 2025
View reviewed changes
Comment threadexir/emit/_emitter.py
Comment on lines +978 to +980
2. et_copy_index(y_outputs, combine_fn's y output, iter_idx)

This explicit copy approach is used because in-place op.out(x, out=x) is unsafe.

@larryliu0820larryliu0820Dec 10, 2025

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.

I was under the impression that this might be fine. We basically emit scan at the very end of the lowering process and I'm not convinced we still require the graph to be functional.

@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No the problem isnt being functional its that aten (and ET ops) are not guaranteed to work when in and out alias the same memory.

You could very easily write before read over sections of the tensor.

Comment threadexir/emit/_emitter.py Outdated
Comment threadexir/emit/_emitter.py Outdated
JacobSzwejbka
JacobSzwejbka commented Dec 10, 2025
View reviewed changes
Comment threadexir/pass_base.py
meta,
)

def call_scan(

@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@angelayi can you check that Im not doing anything stupid here

JacobSzwejbka
JacobSzwejbka commented Dec 10, 2025
View reviewed changes
Comment threadexir/pass_base.py
# Use the placeholder's val which has the correct shape
xs_element_data.append(ph.meta["val"])

combine_fn_result = self.call_submodule(

@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I mostly copied torch.cond here with running call_submodul. Is this just so the subgraph also gets a chance to be run over by spec prop before callign the original? It just seems weird Im calling scan on this subgraph instead of the original one passed in as an arg

@JacobSzwejbka
JacobSzwejbka merged commit fae5d1b into mainDec 11, 2025
164 of 166 checks passed
@JacobSzwejbka
JacobSzwejbka deleted the scan_support branch December 11, 2025 17:41
angelayi
angelayi reviewed Dec 11, 2025
View reviewed changes
Comment threadexir/pass_base.py
for i in range(0, len(xs)):
ph = combine_fn_placeholders[num_init + i]
# Use the placeholder's val which has the correct shape
xs_element_data.append(ph.meta["val"])

@angelayiangelayiDec 11, 2025

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.

i think this part is a little sus where you look at the subgraph's placeholder nodes. I think the xs_element_data should just be something like, xs[0]?

xingguo01 pushed a commit to xingguo01/executorch that referenced this pull request Dec 18, 2025
@JacobSzwejbka@xingguo01
Scan support (pytorch#16028)
907cac4
Add support for higher order ops scan. Its inefficient today because we
are manually deep copying from output to input for every carry. We could
do better by shallow swapping the pointers but Ill do that in a follow
up if needed.
Test plan: Unit tests and internal verification against harder patterns
jirioc pushed a commit to nxp-upstream/executorch that referenced this pull request Dec 19, 2025
@JacobSzwejbka@jirioc
Scan support (pytorch#16028)
1d19628
Add support for higher order ops scan. Its inefficient today because we
are manually deep copying from output to input for every carry. We could
do better by shallow swapping the pointers but Ill do that in a follow
up if needed.
Test plan: Unit tests and internal verification against harder patterns
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@angelayiangelayiangelayi left review comments
@larryliu0820larryliu0820larryliu0820 approved these changes
@manuelcandalesmanuelcandalesAwaiting requested review from manuelcandalesmanuelcandales is a code owner
@lucylqlucylqAwaiting requested review from lucylq
@jackzhxngjackzhxngAwaiting requested review from jackzhxng

Assignees

No one assigned

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JacobSzwejbka@larryliu0820@angelayi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Scan support - #16028
Merged
JacobSzwejbka merged 20 commits into
mainfrom 
scan_support
Dec 11, 2025
Merged
Scan support#16028
JacobSzwejbka merged 20 commits into
mainfrom 
scan_support
Conversation
@JacobSzwejbka
@JacobSzwejbkaJacobSzwejbka
commented
Dec 1, 2025

Copy link


Copy Markdown

Contributor
Add support for higher order ops scan. Its inefficient today because we are manually deep copying from output to input for every carry. We could do better by shallow swapping the pointers but Ill do that in a follow up if needed.
Test plan: Unit tests and internal verification against harder patterns
@pytorch-bot
pytorch-botBot
commented
Dec 1, 2025•

edited

Loading

Copy link


Copy Markdown

🔗 Helpful Links
🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/16028
Note: Links to docs will display an error until the docs builds have been completed.
❌ 1 New Failure, 1 Unrelated Failure
As of commit 90e55dd with merge base 9eaea4a (image):
NEW FAILURE - The following job has failed:
UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes.
@meta-clameta-claBot
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
Dec 1, 2025
@github-actions
github-actionsBot
commented
Dec 1, 2025

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.
@meta-codesync
meta-codesyncBot
commented
Dec 1, 2025

Copy link


Copy Markdown

Contributor
@JacobSzwejbka has imported this pull request. If you are a Meta employee, you can view this in D88107948.
@JacobSzwejbkaJacobSzwejbka
changed the title
[WIP] Scan supportScan supportDec 9, 2025
larryliu0820
larryliu0820
reviewed
Dec 9, 2025
View reviewed changes
Comment threadexir/emit/test/test_emit.py
 op_table = program.execution_plan[0].operators
 instructions = program.execution_plan[0].chains[0].instructions

 # Collect all operator names in the program
@larryliu0820larryliu0820Dec 9, 2025

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.

honestly all the ops seem like implementation details and should not be tested
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I was using it as a sort of a proxy that the general pattern was emitted. If you want we can just test the end 2 end behavior though.
@larryliu0820larryliu0820Dec 10, 2025

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.

Don't have a strong opinion, but you might have to maintain this test if there's a change to the exported graph in the future
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The ops we are querying over are the ones /not/ in the original model definition but instead created by the emitter to maintain the semantics of scan
larryliu0820
larryliu0820
approved these changes
Dec 10, 2025
View reviewed changes
Comment threadexir/emit/_emitter.py

Comment on lines

+978
 to 
+980

 2. et_copy_index(y_outputs, combine_fn's y output, iter_idx)

 This explicit copy approach is used because in-place op.out(x, out=x) is unsafe.
@larryliu0820larryliu0820Dec 10, 2025

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.

I was under the impression that this might be fine. We basically emit scan at the very end of the lowering process and I'm not convinced we still require the graph to be functional.
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

No the problem isnt being functional its that aten (and ET ops) are not guaranteed to work when in and out alias the same memory.
You could very easily write before read over sections of the tensor.
Comment threadexir/emit/_emitter.py
Outdated

Comment threadexir/emit/_emitter.py
Outdated

JacobSzwejbka
JacobSzwejbka
commented
Dec 10, 2025
View reviewed changes
Comment threadexir/pass_base.py
 meta,
 )

 def call_scan(
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@angelayi can you check that Im not doing anything stupid here
JacobSzwejbka
JacobSzwejbka
commented
Dec 10, 2025
View reviewed changes
Comment threadexir/pass_base.py
 # Use the placeholder's val which has the correct shape
 xs_element_data.append(ph.meta["val"])

 combine_fn_result = self.call_submodule(
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I mostly copied torch.cond here with running call_submodul. Is this just so the subgraph also gets a chance to be run over by spec prop before callign the original? It just seems weird Im calling scan on this subgraph instead of the original one passed in as an arg
@JacobSzwejbka
JacobSzwejbka
merged commit fae5d1b
into
mainDec 11, 2025

164 of 166 checks passed

@JacobSzwejbka
JacobSzwejbka
deleted the

scan_support

branch
December 11, 2025 17:41
angelayi
angelayi
reviewed
Dec 11, 2025
View reviewed changes
Comment threadexir/pass_base.py
 for i in range(0, len(xs)):
 ph = combine_fn_placeholders[num_init + i]
 # Use the placeholder's val which has the correct shape
 xs_element_data.append(ph.meta["val"])
@angelayiangelayiDec 11, 2025

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.

i think this part is a little sus where you look at the subgraph's placeholder nodes. I think the xs_element_data should just be something like, xs[0]?
xingguo01 pushed a commit
to xingguo01/executorch
that referenced
this pull request
Dec 18, 2025
@JacobSzwejbka@xingguo01
Scan support (pytorch#16028)
907cac4
Add support for higher order ops scan. Its inefficient today because we
are manually deep copying from output to input for every carry. We could
do better by shallow swapping the pointers but Ill do that in a follow
up if needed.
Test plan: Unit tests and internal verification against harder patterns
jirioc pushed a commit
to nxp-upstream/executorch
that referenced
this pull request
Dec 19, 2025
@JacobSzwejbka@jirioc
Scan support (pytorch#16028)
1d19628
Add support for higher order ops scan. Its inefficient today because we
are manually deep copying from output to input for every carry. We could
do better by shallow swapping the pointers but Ill do that in a follow
up if needed.
Test plan: Unit tests and internal verification against harder patterns
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@angelayiangelayiangelayi left review comments
@larryliu0820larryliu0820larryliu0820 approved these changes
@manuelcandalesmanuelcandalesAwaiting requested review from manuelcandalesmanuelcandales is a code owner
@lucylqlucylqAwaiting requested review from lucylq
@jackzhxngjackzhxngAwaiting requested review from jackzhxng

Assignees


No one assigned


Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@JacobSzwejbka@larryliu0820@angelayi

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Scan support - #16028
Merged
JacobSzwejbka merged 20 commits into
mainfrom 
scan_support
Dec 11, 2025
Merged
Scan support#16028
JacobSzwejbka merged 20 commits into
mainfrom 
scan_support
Conversation
@JacobSzwejbka
@JacobSzwejbkaJacobSzwejbka
commented
Dec 1, 2025

Copy link


Copy Markdown

Contributor
Add support for higher order ops scan. Its inefficient today because we are manually deep copying from output to input for every carry. We could do better by shallow swapping the pointers but Ill do that in a follow up if needed.
Test plan: Unit tests and internal verification against harder patterns
@pytorch-bot
pytorch-botBot
commented
Dec 1, 2025•

edited

Loading

Copy link


Copy Markdown

🔗 Helpful Links
🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/16028
Note: Links to docs will display an error until the docs builds have been completed.
❌ 1 New Failure, 1 Unrelated Failure
As of commit 90e55dd with merge base 9eaea4a (image):
NEW FAILURE - The following job has failed:
UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes.
@meta-clameta-claBot
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
Dec 1, 2025
@github-actions
github-actionsBot
commented
Dec 1, 2025

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.
@meta-codesync
meta-codesyncBot
commented
Dec 1, 2025

Copy link


Copy Markdown

Contributor
@JacobSzwejbka has imported this pull request. If you are a Meta employee, you can view this in D88107948.
@JacobSzwejbkaJacobSzwejbka
changed the title
[WIP] Scan supportScan supportDec 9, 2025
larryliu0820
larryliu0820
reviewed
Dec 9, 2025
View reviewed changes
Comment threadexir/emit/test/test_emit.py
 op_table = program.execution_plan[0].operators
 instructions = program.execution_plan[0].chains[0].instructions

 # Collect all operator names in the program
@larryliu0820larryliu0820Dec 9, 2025

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.

honestly all the ops seem like implementation details and should not be tested
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I was using it as a sort of a proxy that the general pattern was emitted. If you want we can just test the end 2 end behavior though.
@larryliu0820larryliu0820Dec 10, 2025

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.

Don't have a strong opinion, but you might have to maintain this test if there's a change to the exported graph in the future
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The ops we are querying over are the ones /not/ in the original model definition but instead created by the emitter to maintain the semantics of scan
larryliu0820
larryliu0820
approved these changes
Dec 10, 2025
View reviewed changes
Comment threadexir/emit/_emitter.py

Comment on lines

+978
 to 
+980

 2. et_copy_index(y_outputs, combine_fn's y output, iter_idx)

 This explicit copy approach is used because in-place op.out(x, out=x) is unsafe.
@larryliu0820larryliu0820Dec 10, 2025

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.

I was under the impression that this might be fine. We basically emit scan at the very end of the lowering process and I'm not convinced we still require the graph to be functional.
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

No the problem isnt being functional its that aten (and ET ops) are not guaranteed to work when in and out alias the same memory.
You could very easily write before read over sections of the tensor.
Comment threadexir/emit/_emitter.py
Outdated

Comment threadexir/emit/_emitter.py
Outdated

JacobSzwejbka
JacobSzwejbka
commented
Dec 10, 2025
View reviewed changes
Comment threadexir/pass_base.py
 meta,
 )

 def call_scan(
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@angelayi can you check that Im not doing anything stupid here
JacobSzwejbka
JacobSzwejbka
commented
Dec 10, 2025
View reviewed changes
Comment threadexir/pass_base.py
 # Use the placeholder's val which has the correct shape
 xs_element_data.append(ph.meta["val"])

 combine_fn_result = self.call_submodule(
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I mostly copied torch.cond here with running call_submodul. Is this just so the subgraph also gets a chance to be run over by spec prop before callign the original? It just seems weird Im calling scan on this subgraph instead of the original one passed in as an arg
@JacobSzwejbka
JacobSzwejbka
merged commit fae5d1b
into
mainDec 11, 2025

164 of 166 checks passed

@JacobSzwejbka
JacobSzwejbka
deleted the

scan_support

branch
December 11, 2025 17:41
angelayi
angelayi
reviewed
Dec 11, 2025
View reviewed changes
Comment threadexir/pass_base.py
 for i in range(0, len(xs)):
 ph = combine_fn_placeholders[num_init + i]
 # Use the placeholder's val which has the correct shape
 xs_element_data.append(ph.meta["val"])
@angelayiangelayiDec 11, 2025

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.

i think this part is a little sus where you look at the subgraph's placeholder nodes. I think the xs_element_data should just be something like, xs[0]?
xingguo01 pushed a commit
to xingguo01/executorch
that referenced
this pull request
Dec 18, 2025
@JacobSzwejbka@xingguo01
Scan support (pytorch#16028)
907cac4
Add support for higher order ops scan. Its inefficient today because we
are manually deep copying from output to input for every carry. We could
do better by shallow swapping the pointers but Ill do that in a follow
up if needed.
Test plan: Unit tests and internal verification against harder patterns
jirioc pushed a commit
to nxp-upstream/executorch
that referenced
this pull request
Dec 19, 2025
@JacobSzwejbka@jirioc
Scan support (pytorch#16028)
1d19628
Add support for higher order ops scan. Its inefficient today because we
are manually deep copying from output to input for every carry. We could
do better by shallow swapping the pointers but Ill do that in a follow
up if needed.
Test plan: Unit tests and internal verification against harder patterns
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@angelayiangelayiangelayi left review comments
@larryliu0820larryliu0820larryliu0820 approved these changes
@manuelcandalesmanuelcandalesAwaiting requested review from manuelcandalesmanuelcandales is a code owner
@lucylqlucylqAwaiting requested review from lucylq
@jackzhxngjackzhxngAwaiting requested review from jackzhxng

Assignees


No one assigned


Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@JacobSzwejbka@larryliu0820@angelayi

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Scan support - #16028
Merged
JacobSzwejbka merged 20 commits into
mainfrom 
scan_support
Dec 11, 2025
Merged
Scan support#16028
JacobSzwejbka merged 20 commits into
mainfrom 
scan_support
Conversation
@JacobSzwejbka
@JacobSzwejbkaJacobSzwejbka
commented
Dec 1, 2025

Copy link


Copy Markdown

Contributor
Add support for higher order ops scan. Its inefficient today because we are manually deep copying from output to input for every carry. We could do better by shallow swapping the pointers but Ill do that in a follow up if needed.
Test plan: Unit tests and internal verification against harder patterns
@pytorch-bot
pytorch-botBot
commented
Dec 1, 2025•

edited

Loading

Copy link


Copy Markdown

🔗 Helpful Links
🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/16028
Note: Links to docs will display an error until the docs builds have been completed.
❌ 1 New Failure, 1 Unrelated Failure
As of commit 90e55dd with merge base 9eaea4a (image):
NEW FAILURE - The following job has failed:
UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes.
@meta-clameta-claBot
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
Dec 1, 2025
@github-actions
github-actionsBot
commented
Dec 1, 2025

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.
@meta-codesync
meta-codesyncBot
commented
Dec 1, 2025

Copy link


Copy Markdown

Contributor
@JacobSzwejbka has imported this pull request. If you are a Meta employee, you can view this in D88107948.
@JacobSzwejbkaJacobSzwejbka
changed the title
[WIP] Scan supportScan supportDec 9, 2025
larryliu0820
larryliu0820
reviewed
Dec 9, 2025
View reviewed changes
Comment threadexir/emit/test/test_emit.py
 op_table = program.execution_plan[0].operators
 instructions = program.execution_plan[0].chains[0].instructions

 # Collect all operator names in the program
@larryliu0820larryliu0820Dec 9, 2025

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.

honestly all the ops seem like implementation details and should not be tested
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I was using it as a sort of a proxy that the general pattern was emitted. If you want we can just test the end 2 end behavior though.
@larryliu0820larryliu0820Dec 10, 2025

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.

Don't have a strong opinion, but you might have to maintain this test if there's a change to the exported graph in the future
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The ops we are querying over are the ones /not/ in the original model definition but instead created by the emitter to maintain the semantics of scan
larryliu0820
larryliu0820
approved these changes
Dec 10, 2025
View reviewed changes
Comment threadexir/emit/_emitter.py

Comment on lines

+978
 to 
+980

 2. et_copy_index(y_outputs, combine_fn's y output, iter_idx)

 This explicit copy approach is used because in-place op.out(x, out=x) is unsafe.
@larryliu0820larryliu0820Dec 10, 2025

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.

I was under the impression that this might be fine. We basically emit scan at the very end of the lowering process and I'm not convinced we still require the graph to be functional.
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

No the problem isnt being functional its that aten (and ET ops) are not guaranteed to work when in and out alias the same memory.
You could very easily write before read over sections of the tensor.
Comment threadexir/emit/_emitter.py
Outdated

Comment threadexir/emit/_emitter.py
Outdated

JacobSzwejbka
JacobSzwejbka
commented
Dec 10, 2025
View reviewed changes
Comment threadexir/pass_base.py
 meta,
 )

 def call_scan(
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@angelayi can you check that Im not doing anything stupid here
JacobSzwejbka
JacobSzwejbka
commented
Dec 10, 2025
View reviewed changes
Comment threadexir/pass_base.py
 # Use the placeholder's val which has the correct shape
 xs_element_data.append(ph.meta["val"])

 combine_fn_result = self.call_submodule(
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I mostly copied torch.cond here with running call_submodul. Is this just so the subgraph also gets a chance to be run over by spec prop before callign the original? It just seems weird Im calling scan on this subgraph instead of the original one passed in as an arg
@JacobSzwejbka
JacobSzwejbka
merged commit fae5d1b
into
mainDec 11, 2025

164 of 166 checks passed

@JacobSzwejbka
JacobSzwejbka
deleted the

scan_support

branch
December 11, 2025 17:41
angelayi
angelayi
reviewed
Dec 11, 2025
View reviewed changes
Comment threadexir/pass_base.py
 for i in range(0, len(xs)):
 ph = combine_fn_placeholders[num_init + i]
 # Use the placeholder's val which has the correct shape
 xs_element_data.append(ph.meta["val"])
@angelayiangelayiDec 11, 2025

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.

i think this part is a little sus where you look at the subgraph's placeholder nodes. I think the xs_element_data should just be something like, xs[0]?
xingguo01 pushed a commit
to xingguo01/executorch
that referenced
this pull request
Dec 18, 2025
@JacobSzwejbka@xingguo01
Scan support (pytorch#16028)
907cac4
Add support for higher order ops scan. Its inefficient today because we
are manually deep copying from output to input for every carry. We could
do better by shallow swapping the pointers but Ill do that in a follow
up if needed.
Test plan: Unit tests and internal verification against harder patterns
jirioc pushed a commit
to nxp-upstream/executorch
that referenced
this pull request
Dec 19, 2025
@JacobSzwejbka@jirioc
Scan support (pytorch#16028)
1d19628
Add support for higher order ops scan. Its inefficient today because we
are manually deep copying from output to input for every carry. We could
do better by shallow swapping the pointers but Ill do that in a follow
up if needed.
Test plan: Unit tests and internal verification against harder patterns
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@angelayiangelayiangelayi left review comments
@larryliu0820larryliu0820larryliu0820 approved these changes
@manuelcandalesmanuelcandalesAwaiting requested review from manuelcandalesmanuelcandales is a code owner
@lucylqlucylqAwaiting requested review from lucylq
@jackzhxngjackzhxngAwaiting requested review from jackzhxng

Assignees


No one assigned


Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@JacobSzwejbka@larryliu0820@angelayi

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Scan support - #16028
Merged
JacobSzwejbka merged 20 commits into
mainfrom 
scan_support
Dec 11, 2025
Merged
Scan support#16028
JacobSzwejbka merged 20 commits into
mainfrom 
scan_support
Conversation
@JacobSzwejbka
@JacobSzwejbkaJacobSzwejbka
commented
Dec 1, 2025

Copy link


Copy Markdown

Contributor
Add support for higher order ops scan. Its inefficient today because we are manually deep copying from output to input for every carry. We could do better by shallow swapping the pointers but Ill do that in a follow up if needed.
Test plan: Unit tests and internal verification against harder patterns
@pytorch-bot
pytorch-botBot
commented
Dec 1, 2025•

edited

Loading

Copy link


Copy Markdown

🔗 Helpful Links
🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/16028
Note: Links to docs will display an error until the docs builds have been completed.
❌ 1 New Failure, 1 Unrelated Failure
As of commit 90e55dd with merge base 9eaea4a (image):
NEW FAILURE - The following job has failed:
UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes.
@meta-clameta-claBot
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
Dec 1, 2025
@github-actions
github-actionsBot
commented
Dec 1, 2025

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.
@meta-codesync
meta-codesyncBot
commented
Dec 1, 2025

Copy link


Copy Markdown

Contributor
@JacobSzwejbka has imported this pull request. If you are a Meta employee, you can view this in D88107948.
@JacobSzwejbkaJacobSzwejbka
changed the title
[WIP] Scan supportScan supportDec 9, 2025
larryliu0820
larryliu0820
reviewed
Dec 9, 2025
View reviewed changes
Comment threadexir/emit/test/test_emit.py
 op_table = program.execution_plan[0].operators
 instructions = program.execution_plan[0].chains[0].instructions

 # Collect all operator names in the program
@larryliu0820larryliu0820Dec 9, 2025

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.

honestly all the ops seem like implementation details and should not be tested
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I was using it as a sort of a proxy that the general pattern was emitted. If you want we can just test the end 2 end behavior though.
@larryliu0820larryliu0820Dec 10, 2025

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.

Don't have a strong opinion, but you might have to maintain this test if there's a change to the exported graph in the future
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The ops we are querying over are the ones /not/ in the original model definition but instead created by the emitter to maintain the semantics of scan
larryliu0820
larryliu0820
approved these changes
Dec 10, 2025
View reviewed changes
Comment threadexir/emit/_emitter.py

Comment on lines

+978
 to 
+980

 2. et_copy_index(y_outputs, combine_fn's y output, iter_idx)

 This explicit copy approach is used because in-place op.out(x, out=x) is unsafe.
@larryliu0820larryliu0820Dec 10, 2025

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.

I was under the impression that this might be fine. We basically emit scan at the very end of the lowering process and I'm not convinced we still require the graph to be functional.
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

No the problem isnt being functional its that aten (and ET ops) are not guaranteed to work when in and out alias the same memory.
You could very easily write before read over sections of the tensor.
Comment threadexir/emit/_emitter.py
Outdated

Comment threadexir/emit/_emitter.py
Outdated

JacobSzwejbka
JacobSzwejbka
commented
Dec 10, 2025
View reviewed changes
Comment threadexir/pass_base.py
 meta,
 )

 def call_scan(
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@angelayi can you check that Im not doing anything stupid here
JacobSzwejbka
JacobSzwejbka
commented
Dec 10, 2025
View reviewed changes
Comment threadexir/pass_base.py
 # Use the placeholder's val which has the correct shape
 xs_element_data.append(ph.meta["val"])

 combine_fn_result = self.call_submodule(
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I mostly copied torch.cond here with running call_submodul. Is this just so the subgraph also gets a chance to be run over by spec prop before callign the original? It just seems weird Im calling scan on this subgraph instead of the original one passed in as an arg
@JacobSzwejbka
JacobSzwejbka
merged commit fae5d1b
into
mainDec 11, 2025

164 of 166 checks passed

@JacobSzwejbka
JacobSzwejbka
deleted the

scan_support

branch
December 11, 2025 17:41
angelayi
angelayi
reviewed
Dec 11, 2025
View reviewed changes
Comment threadexir/pass_base.py
 for i in range(0, len(xs)):
 ph = combine_fn_placeholders[num_init + i]
 # Use the placeholder's val which has the correct shape
 xs_element_data.append(ph.meta["val"])
@angelayiangelayiDec 11, 2025

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.

i think this part is a little sus where you look at the subgraph's placeholder nodes. I think the xs_element_data should just be something like, xs[0]?
xingguo01 pushed a commit
to xingguo01/executorch
that referenced
this pull request
Dec 18, 2025
@JacobSzwejbka@xingguo01
Scan support (pytorch#16028)
907cac4
Add support for higher order ops scan. Its inefficient today because we
are manually deep copying from output to input for every carry. We could
do better by shallow swapping the pointers but Ill do that in a follow
up if needed.
Test plan: Unit tests and internal verification against harder patterns
jirioc pushed a commit
to nxp-upstream/executorch
that referenced
this pull request
Dec 19, 2025
@JacobSzwejbka@jirioc
Scan support (pytorch#16028)
1d19628
Add support for higher order ops scan. Its inefficient today because we
are manually deep copying from output to input for every carry. We could
do better by shallow swapping the pointers but Ill do that in a follow
up if needed.
Test plan: Unit tests and internal verification against harder patterns
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@angelayiangelayiangelayi left review comments
@larryliu0820larryliu0820larryliu0820 approved these changes
@manuelcandalesmanuelcandalesAwaiting requested review from manuelcandalesmanuelcandales is a code owner
@lucylqlucylqAwaiting requested review from lucylq
@jackzhxngjackzhxngAwaiting requested review from jackzhxng

Assignees


No one assigned


Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@JacobSzwejbka@larryliu0820@angelayi

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Scan support - #16028
Merged
JacobSzwejbka merged 20 commits into
mainfrom 
scan_support
Dec 11, 2025
Merged
Scan support#16028
JacobSzwejbka merged 20 commits into
mainfrom 
scan_support
Conversation
@JacobSzwejbka
@JacobSzwejbkaJacobSzwejbka
commented
Dec 1, 2025

Copy link


Copy Markdown

Contributor
Add support for higher order ops scan. Its inefficient today because we are manually deep copying from output to input for every carry. We could do better by shallow swapping the pointers but Ill do that in a follow up if needed.
Test plan: Unit tests and internal verification against harder patterns
@pytorch-bot
pytorch-botBot
commented
Dec 1, 2025•

edited

Loading

Copy link


Copy Markdown

🔗 Helpful Links
🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/16028
Note: Links to docs will display an error until the docs builds have been completed.
❌ 1 New Failure, 1 Unrelated Failure
As of commit 90e55dd with merge base 9eaea4a (image):
NEW FAILURE - The following job has failed:
UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes.
@meta-clameta-claBot
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
Dec 1, 2025
@github-actions
github-actionsBot
commented
Dec 1, 2025

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.
@meta-codesync
meta-codesyncBot
commented
Dec 1, 2025

Copy link


Copy Markdown

Contributor
@JacobSzwejbka has imported this pull request. If you are a Meta employee, you can view this in D88107948.
@JacobSzwejbkaJacobSzwejbka
changed the title
[WIP] Scan supportScan supportDec 9, 2025
larryliu0820
larryliu0820
reviewed
Dec 9, 2025
View reviewed changes
Comment threadexir/emit/test/test_emit.py
 op_table = program.execution_plan[0].operators
 instructions = program.execution_plan[0].chains[0].instructions

 # Collect all operator names in the program
@larryliu0820larryliu0820Dec 9, 2025

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.

honestly all the ops seem like implementation details and should not be tested
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I was using it as a sort of a proxy that the general pattern was emitted. If you want we can just test the end 2 end behavior though.
@larryliu0820larryliu0820Dec 10, 2025

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.

Don't have a strong opinion, but you might have to maintain this test if there's a change to the exported graph in the future
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The ops we are querying over are the ones /not/ in the original model definition but instead created by the emitter to maintain the semantics of scan
larryliu0820
larryliu0820
approved these changes
Dec 10, 2025
View reviewed changes
Comment threadexir/emit/_emitter.py

Comment on lines

+978
 to 
+980

 2. et_copy_index(y_outputs, combine_fn's y output, iter_idx)

 This explicit copy approach is used because in-place op.out(x, out=x) is unsafe.
@larryliu0820larryliu0820Dec 10, 2025

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.

I was under the impression that this might be fine. We basically emit scan at the very end of the lowering process and I'm not convinced we still require the graph to be functional.
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

No the problem isnt being functional its that aten (and ET ops) are not guaranteed to work when in and out alias the same memory.
You could very easily write before read over sections of the tensor.
Comment threadexir/emit/_emitter.py
Outdated

Comment threadexir/emit/_emitter.py
Outdated

JacobSzwejbka
JacobSzwejbka
commented
Dec 10, 2025
View reviewed changes
Comment threadexir/pass_base.py
 meta,
 )

 def call_scan(
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@angelayi can you check that Im not doing anything stupid here
JacobSzwejbka
JacobSzwejbka
commented
Dec 10, 2025
View reviewed changes
Comment threadexir/pass_base.py
 # Use the placeholder's val which has the correct shape
 xs_element_data.append(ph.meta["val"])

 combine_fn_result = self.call_submodule(
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I mostly copied torch.cond here with running call_submodul. Is this just so the subgraph also gets a chance to be run over by spec prop before callign the original? It just seems weird Im calling scan on this subgraph instead of the original one passed in as an arg
@JacobSzwejbka
JacobSzwejbka
merged commit fae5d1b
into
mainDec 11, 2025

164 of 166 checks passed

@JacobSzwejbka
JacobSzwejbka
deleted the

scan_support

branch
December 11, 2025 17:41
angelayi
angelayi
reviewed
Dec 11, 2025
View reviewed changes
Comment threadexir/pass_base.py
 for i in range(0, len(xs)):
 ph = combine_fn_placeholders[num_init + i]
 # Use the placeholder's val which has the correct shape
 xs_element_data.append(ph.meta["val"])
@angelayiangelayiDec 11, 2025

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.

i think this part is a little sus where you look at the subgraph's placeholder nodes. I think the xs_element_data should just be something like, xs[0]?
xingguo01 pushed a commit
to xingguo01/executorch
that referenced
this pull request
Dec 18, 2025
@JacobSzwejbka@xingguo01
Scan support (pytorch#16028)
907cac4
Add support for higher order ops scan. Its inefficient today because we
are manually deep copying from output to input for every carry. We could
do better by shallow swapping the pointers but Ill do that in a follow
up if needed.
Test plan: Unit tests and internal verification against harder patterns
jirioc pushed a commit
to nxp-upstream/executorch
that referenced
this pull request
Dec 19, 2025
@JacobSzwejbka@jirioc
Scan support (pytorch#16028)
1d19628
Add support for higher order ops scan. Its inefficient today because we
are manually deep copying from output to input for every carry. We could
do better by shallow swapping the pointers but Ill do that in a follow
up if needed.
Test plan: Unit tests and internal verification against harder patterns
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@angelayiangelayiangelayi left review comments
@larryliu0820larryliu0820larryliu0820 approved these changes
@manuelcandalesmanuelcandalesAwaiting requested review from manuelcandalesmanuelcandales is a code owner
@lucylqlucylqAwaiting requested review from lucylq
@jackzhxngjackzhxngAwaiting requested review from jackzhxng

Assignees


No one assigned


Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@JacobSzwejbka@larryliu0820@angelayi

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Scan support - #16028
Merged
JacobSzwejbka merged 20 commits into
mainfrom 
scan_support
Dec 11, 2025
Merged
Scan support#16028
JacobSzwejbka merged 20 commits into
mainfrom 
scan_support
Conversation
@JacobSzwejbka
@JacobSzwejbkaJacobSzwejbka
commented
Dec 1, 2025

Copy link


Copy Markdown

Contributor
Add support for higher order ops scan. Its inefficient today because we are manually deep copying from output to input for every carry. We could do better by shallow swapping the pointers but Ill do that in a follow up if needed.
Test plan: Unit tests and internal verification against harder patterns
@pytorch-bot
pytorch-botBot
commented
Dec 1, 2025•

edited

Loading

Copy link


Copy Markdown

🔗 Helpful Links
🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/16028
Note: Links to docs will display an error until the docs builds have been completed.
❌ 1 New Failure, 1 Unrelated Failure
As of commit 90e55dd with merge base 9eaea4a (image):
NEW FAILURE - The following job has failed:
UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes.
@meta-clameta-claBot
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
Dec 1, 2025
@github-actions
github-actionsBot
commented
Dec 1, 2025

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.
@meta-codesync
meta-codesyncBot
commented
Dec 1, 2025

Copy link


Copy Markdown

Contributor
@JacobSzwejbka has imported this pull request. If you are a Meta employee, you can view this in D88107948.
@JacobSzwejbkaJacobSzwejbka
changed the title
[WIP] Scan supportScan supportDec 9, 2025
larryliu0820
larryliu0820
reviewed
Dec 9, 2025
View reviewed changes
Comment threadexir/emit/test/test_emit.py
 op_table = program.execution_plan[0].operators
 instructions = program.execution_plan[0].chains[0].instructions

 # Collect all operator names in the program
@larryliu0820larryliu0820Dec 9, 2025

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.

honestly all the ops seem like implementation details and should not be tested
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I was using it as a sort of a proxy that the general pattern was emitted. If you want we can just test the end 2 end behavior though.
@larryliu0820larryliu0820Dec 10, 2025

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.

Don't have a strong opinion, but you might have to maintain this test if there's a change to the exported graph in the future
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The ops we are querying over are the ones /not/ in the original model definition but instead created by the emitter to maintain the semantics of scan
larryliu0820
larryliu0820
approved these changes
Dec 10, 2025
View reviewed changes
Comment threadexir/emit/_emitter.py

Comment on lines

+978
 to 
+980

 2. et_copy_index(y_outputs, combine_fn's y output, iter_idx)

 This explicit copy approach is used because in-place op.out(x, out=x) is unsafe.
@larryliu0820larryliu0820Dec 10, 2025

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.

I was under the impression that this might be fine. We basically emit scan at the very end of the lowering process and I'm not convinced we still require the graph to be functional.
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

No the problem isnt being functional its that aten (and ET ops) are not guaranteed to work when in and out alias the same memory.
You could very easily write before read over sections of the tensor.
Comment threadexir/emit/_emitter.py
Outdated

Comment threadexir/emit/_emitter.py
Outdated

JacobSzwejbka
JacobSzwejbka
commented
Dec 10, 2025
View reviewed changes
Comment threadexir/pass_base.py
 meta,
 )

 def call_scan(
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@angelayi can you check that Im not doing anything stupid here
JacobSzwejbka
JacobSzwejbka
commented
Dec 10, 2025
View reviewed changes
Comment threadexir/pass_base.py
 # Use the placeholder's val which has the correct shape
 xs_element_data.append(ph.meta["val"])

 combine_fn_result = self.call_submodule(
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I mostly copied torch.cond here with running call_submodul. Is this just so the subgraph also gets a chance to be run over by spec prop before callign the original? It just seems weird Im calling scan on this subgraph instead of the original one passed in as an arg
@JacobSzwejbka
JacobSzwejbka
merged commit fae5d1b
into
mainDec 11, 2025

164 of 166 checks passed

@JacobSzwejbka
JacobSzwejbka
deleted the

scan_support

branch
December 11, 2025 17:41
angelayi
angelayi
reviewed
Dec 11, 2025
View reviewed changes
Comment threadexir/pass_base.py
 for i in range(0, len(xs)):
 ph = combine_fn_placeholders[num_init + i]
 # Use the placeholder's val which has the correct shape
 xs_element_data.append(ph.meta["val"])
@angelayiangelayiDec 11, 2025

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.

i think this part is a little sus where you look at the subgraph's placeholder nodes. I think the xs_element_data should just be something like, xs[0]?
xingguo01 pushed a commit
to xingguo01/executorch
that referenced
this pull request
Dec 18, 2025
@JacobSzwejbka@xingguo01
Scan support (pytorch#16028)
907cac4
Add support for higher order ops scan. Its inefficient today because we
are manually deep copying from output to input for every carry. We could
do better by shallow swapping the pointers but Ill do that in a follow
up if needed.
Test plan: Unit tests and internal verification against harder patterns
jirioc pushed a commit
to nxp-upstream/executorch
that referenced
this pull request
Dec 19, 2025
@JacobSzwejbka@jirioc
Scan support (pytorch#16028)
1d19628
Add support for higher order ops scan. Its inefficient today because we
are manually deep copying from output to input for every carry. We could
do better by shallow swapping the pointers but Ill do that in a follow
up if needed.
Test plan: Unit tests and internal verification against harder patterns
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@angelayiangelayiangelayi left review comments
@larryliu0820larryliu0820larryliu0820 approved these changes
@manuelcandalesmanuelcandalesAwaiting requested review from manuelcandalesmanuelcandales is a code owner
@lucylqlucylqAwaiting requested review from lucylq
@jackzhxngjackzhxngAwaiting requested review from jackzhxng

Assignees


No one assigned


Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@JacobSzwejbka@larryliu0820@angelayi

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Scan support - #16028
Merged
JacobSzwejbka merged 20 commits into
mainfrom 
scan_support
Dec 11, 2025
Merged
Scan support#16028
JacobSzwejbka merged 20 commits into
mainfrom 
scan_support
Conversation
@JacobSzwejbka
@JacobSzwejbkaJacobSzwejbka
commented
Dec 1, 2025

Copy link


Copy Markdown

Contributor
Add support for higher order ops scan. Its inefficient today because we are manually deep copying from output to input for every carry. We could do better by shallow swapping the pointers but Ill do that in a follow up if needed.
Test plan: Unit tests and internal verification against harder patterns
@pytorch-bot
pytorch-botBot
commented
Dec 1, 2025•

edited

Loading

Copy link


Copy Markdown

🔗 Helpful Links
🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/16028
Note: Links to docs will display an error until the docs builds have been completed.
❌ 1 New Failure, 1 Unrelated Failure
As of commit 90e55dd with merge base 9eaea4a (image):
NEW FAILURE - The following job has failed:
UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes.
@meta-clameta-claBot
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
Dec 1, 2025
@github-actions
github-actionsBot
commented
Dec 1, 2025

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.
@meta-codesync
meta-codesyncBot
commented
Dec 1, 2025

Copy link


Copy Markdown

Contributor
@JacobSzwejbka has imported this pull request. If you are a Meta employee, you can view this in D88107948.
@JacobSzwejbkaJacobSzwejbka
changed the title
[WIP] Scan supportScan supportDec 9, 2025
larryliu0820
larryliu0820
reviewed
Dec 9, 2025
View reviewed changes
Comment threadexir/emit/test/test_emit.py
 op_table = program.execution_plan[0].operators
 instructions = program.execution_plan[0].chains[0].instructions

 # Collect all operator names in the program
@larryliu0820larryliu0820Dec 9, 2025

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.

honestly all the ops seem like implementation details and should not be tested
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I was using it as a sort of a proxy that the general pattern was emitted. If you want we can just test the end 2 end behavior though.
@larryliu0820larryliu0820Dec 10, 2025

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.

Don't have a strong opinion, but you might have to maintain this test if there's a change to the exported graph in the future
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The ops we are querying over are the ones /not/ in the original model definition but instead created by the emitter to maintain the semantics of scan
larryliu0820
larryliu0820
approved these changes
Dec 10, 2025
View reviewed changes
Comment threadexir/emit/_emitter.py

Comment on lines

+978
 to 
+980

 2. et_copy_index(y_outputs, combine_fn's y output, iter_idx)

 This explicit copy approach is used because in-place op.out(x, out=x) is unsafe.
@larryliu0820larryliu0820Dec 10, 2025

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.

I was under the impression that this might be fine. We basically emit scan at the very end of the lowering process and I'm not convinced we still require the graph to be functional.
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

No the problem isnt being functional its that aten (and ET ops) are not guaranteed to work when in and out alias the same memory.
You could very easily write before read over sections of the tensor.
Comment threadexir/emit/_emitter.py
Outdated

Comment threadexir/emit/_emitter.py
Outdated

JacobSzwejbka
JacobSzwejbka
commented
Dec 10, 2025
View reviewed changes
Comment threadexir/pass_base.py
 meta,
 )

 def call_scan(
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@angelayi can you check that Im not doing anything stupid here
JacobSzwejbka
JacobSzwejbka
commented
Dec 10, 2025
View reviewed changes
Comment threadexir/pass_base.py
 # Use the placeholder's val which has the correct shape
 xs_element_data.append(ph.meta["val"])

 combine_fn_result = self.call_submodule(
@JacobSzwejbkaJacobSzwejbkaDec 10, 2025

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I mostly copied torch.cond here with running call_submodul. Is this just so the subgraph also gets a chance to be run over by spec prop before callign the original? It just seems weird Im calling scan on this subgraph instead of the original one passed in as an arg
@JacobSzwejbka
JacobSzwejbka
merged commit fae5d1b
into
mainDec 11, 2025

164 of 166 checks passed

@JacobSzwejbka
JacobSzwejbka
deleted the

scan_support

branch
December 11, 2025 17:41
angelayi
angelayi
reviewed
Dec 11, 2025
View reviewed changes
Comment threadexir/pass_base.py
 for i in range(0, len(xs)):
 ph = combine_fn_placeholders[num_init + i]
 # Use the placeholder's val which has the correct shape
 xs_element_data.append(ph.meta["val"])
@angelayiangelayiDec 11, 2025

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.

i think this part is a little sus where you look at the subgraph's placeholder nodes. I think the xs_element_data should just be something like, xs[0]?
xingguo01 pushed a commit
to xingguo01/executorch
that referenced
this pull request
Dec 18, 2025
@JacobSzwejbka@xingguo01
Scan support (pytorch#16028)
907cac4
Add support for higher order ops scan. Its inefficient today because we
are manually deep copying from output to input for every carry. We could
do better by shallow swapping the pointers but Ill do that in a follow
up if needed.
Test plan: Unit tests and internal verification against harder patterns
jirioc pushed a commit
to nxp-upstream/executorch
that referenced
this pull request
Dec 19, 2025
@JacobSzwejbka@jirioc
Scan support (pytorch#16028)
1d19628
Add support for higher order ops scan. Its inefficient today because we
are manually deep copying from output to input for every carry. We could
do better by shallow swapping the pointers but Ill do that in a follow
up if needed.
Test plan: Unit tests and internal verification against harder patterns
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@angelayiangelayiangelayi left review comments
@larryliu0820larryliu0820larryliu0820 approved these changes
@manuelcandalesmanuelcandalesAwaiting requested review from manuelcandalesmanuelcandales is a code owner
@lucylqlucylqAwaiting requested review from lucylq
@jackzhxngjackzhxngAwaiting requested review from jackzhxng

Assignees


No one assigned


Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@JacobSzwejbka@larryliu0820@angelayi