-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Inplace binary operation class for the OpenFHE IR + sum and sub inplace operations #1339
Merged
copybara-service
merged 2 commits into
google:main
from
ShirChoiEX:OpenfheInplaceOperations
Feb 7, 2025
+40
−0
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a recent PR (#1334) that relaxes the
AllTypesMatch
requirement to[SameOperandsAndResultRings, InferTypeOpAdaptor]
Discussion for maintainers: We might also need to register a
MemoryEffects<[MemWrite]>
to make liveness analysis happy, though I found--cse --canonicalize
does not eliminate these inplace ops.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a need to add
[SameOperandsAndResultRings]
trait for the inplace operations? It seems to be analyzing both inputs and the output, but there is no output for an inplace function. There seem to be no alternative trait like[SameOperandsRings]
, or does the[SameOperandsAndResultRings]
ignore the result check if it is not created? Also, regarding the #1334 PR, he uses[SameOperandsAndResultRings]
in AddOp and SubOp but not in the MulOp why is it so?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the question is why no
SameOperandsAndResultRings
is needed foropenfhe.mul
it's becauseAllTypesMatch<["lhs", "rhs", "output"]>
impliesSameOperandsAndResultRings
, but if the question is instead why I didn't "relax" the constraints onopenfhe.mul
to "only"SameOperandsAndResultRings
, that's just because the op isn't used in our default lowering pipeline so there was no need to.EDIT: Note that the
AllTypesMatch<["lhs", "rhs", "output"]>
was already applied toopenfhe.mul
before my PR, it was just previously inherited fromOpenfhe_BinaryOp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the following line we can know that if there is no result, the trait only checks the equivalence among operands.
heir/lib/Dialect/LWE/IR/LWETraits.h
Lines 19 to 50 in ad59e8f