Skip to content

Fix aliasing bug in OpenFHE emitter for InsertOp with shared buffers - #3198

Open
toprakmurat wants to merge 1 commit into
google:mainfrom
toprakmurat:fix/aliasing-openfhe-emitter
Open

Fix aliasing bug in OpenFHE emitter for InsertOp with shared buffers#3198
toprakmurat wants to merge 1 commit into
google:mainfrom
toprakmurat:fix/aliasing-openfhe-emitter

Conversation

@toprakmurat

Copy link
Copy Markdown
Contributor

Fixes #3196

Implemented changes mostly based on printOperation(tensor::InsertSliceOp)

I did not look into Lattigo backend, I can change it with a different PR if needed.

Tested with the test code @VedantParanjape provided.

@github-actions

Copy link
Copy Markdown

Welcome to HEIR!

Thank you for opening your first pull request! We appreciate you joining our community.

Please note that our contributing policy) requires unrecognized users to meet a maintainer in a non-textual format before their first PR will be reviewed.

If you have already met a maintainer, or you're sure the maintainers know who you are, you can safely ignore this message. Otherwise, a maintainer will likely close this PR shortly.

Don't take it personally! Instead, we welcome you to attend one of our open meetings on our community calendar, schedule a dedicated call with a maintainer, or reach out in the #heir channel on Discord.

We look forward to connecting with you!

@VedantParanjape VedantParanjape left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the fix!

@VedantParanjape

Copy link
Copy Markdown
Collaborator

@j2kun could you review this as well?

@j2kun j2kun left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. The lattigo backend is bufferized, meaning there are no tensor ops. I plan on bufferizing the openfhe backend, but I have gotten sidetracked with other priorities.

@j2kun j2kun added the pull_ready Indicates whether a PR is ready to pull. The copybara worker will import for internal testing label Jul 15, 2026
@j2kun

j2kun commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

So I was looking at this in more detail and I think the change is actually not the right thing to do. Indeed, it is semantically wrong, but copy on write will make the resulting FHE programs extremely slow (not even for FHE reasons, just because it will be copying data way too often).

The right solution here is to bufferize the openfhe dialect, so that the operations are memref and memory safety is upheld by the bufferization logic.

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

Labels

pull_ready Indicates whether a PR is ready to pull. The copybara worker will import for internal testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OpenFhePkeEmitter's tensor.insert emission aliases SSA result to destination buffer, collapsing distinct outputs in multi-result functions

3 participants