-
Notifications
You must be signed in to change notification settings - Fork 48
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
Implement ModArithType for mod_arith
dialect
#1088
Implement ModArithType for mod_arith
dialect
#1088
Conversation
IMO this looks great. I think the use of the |
Encountered issues of integer range when trying to migrate HEaaN.mlir resolves such issue by using a
Should we annotate the For this PR. I think migrating basic ops like add/sub/mul with basic lowering is enough. And as barett_reduce is not used for now (though there was a PR #712 doing such transform), |
07b4ac1
to
8a15c51
Compare
tests/Dialect/ModArith/Conversions/mod_arith_to_arith/runner/lower_add.mlir
Show resolved
Hide resolved
Your reasoning is sound for the decision here. From my perspective, the main trade-off here is the inefficiency loss by incurring an extra field on every If we put it on the ops, I just want to quickly sketch out the extra work that would require. Say we want to lower |
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.
Looks fantastic! My main critique is that we should be clear at the start about the operation semantics in the tablegen description fields, as that will be the source of truth when folks (particularly me) forget what we decided on.
tests/Dialect/ModArith/Conversions/mod_arith_to_arith/runner/lower_add.mlir
Show resolved
Hide resolved
lib/Dialect/ModArith/Conversions/ModArithToArith/ModArithToArith.cpp
Outdated
Show resolved
Hide resolved
lib/Dialect/ModArith/Conversions/ModArithToArith/ModArithToArith.cpp
Outdated
Show resolved
Hide resolved
lib/Dialect/ModArith/Conversions/ModArithToArith/ModArithToArith.cpp
Outdated
Show resolved
Hide resolved
8a15c51
to
a8eb769
Compare
auto x = b.create<arith::ExtUIOp>(modulusType(op, true), | ||
adaptor.getOperands()[0]); | ||
auto y = b.create<arith::ExtUIOp>(modulusType(op, true), | ||
adaptor.getOperands()[1]); | ||
auto acc = b.create<arith::ExtUIOp>(modulusType(op, true), | ||
adaptor.getOperands()[2]); |
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.
just for later - the OpAdaptor comes with named accessors for the operands (like adaptor.getLhs(), adaptor.getRhs(), adapator.getAcc())
See #1084
This PR is in draft state, opened here for reviewing the
ModArithType
type itself.It is worth noting is that the type explicitly does not allow modulus value to be too large to fill the entire underlying type as there would be signedness issues. As we are working in FHE situations where modulus width typically goes up to only 60 for 64bit native int, this requirement does not harm the functionality and edge cases for lowering to native int could be avoided.
Later pushes will replace all existing
ModArithOps
to use theModArithType
and correspondingModArithToArith
lowering. Currentlymreduce
andmadd
serves only as a demo for the type verification and would be renamed toreduce
/add
once finished migration.Syntax now: