ArithExt Improvements: Arbitrary Coefficient Bitwidth + lowerings of modular add/sub/mul to arith #731
Conversation
AlexanderViand-Intel
force-pushed
the
arith_ext_improvements
branch
from
7f7743e
to
a0caa5c
Compare
AlexanderViand-Intel
force-pushed
the
arith_ext_improvements
branch
2 times, most recently
from
467a48c
to
645adfe
Compare
| return op.emitOpError() | ||
| << "underlying type's bitwidth must be at least as " | ||
| << "large as the modulus bitwidth, but got " << bitWidth | ||
| << " while modulus requires width " << modWidth << "."; |
There was a problem hiding this comment.
Since it's not typed, we should be careful that a value of 2**32 - 1 is not interpreted as -1, as @asraa mentioned in another comment.
Can we add a verification to assert that, even when interpreting the modulus as signed, the value is always > 0, and then add a test for a modulus of 2**64 - 1 (64 is the default APInt bit width).
There was a problem hiding this comment.
Mh, very good point. I guess we shouldn't just hardcode to unsigned since we want to allow the underlying "container" type to be both signed (for people who prefer $[\floor{-q/2}, \floor{q/2})$) and unsigned (for
I'm not sure we can hard error on the > 0 check in all cases, though - it's a bit contrived, but someone might want their modulus to be 2^31-1 and use a 31-bit underlying type, which should pass the check here?
Maybe we should error if the underlying type is signed and the modulus isn't >0, warn if the underlying type is signless and the modulus would be <0 if interpreted as signed and not emit any warning/error if the underlying type is unsigned?
There was a problem hiding this comment.
but someone might want their modulus to be 2^31-1 and use a 31-bit underlying type, which should pass the check here?
This is where I ran into issues with the remsi/remui before, since if the input is negative and the modulus is positive, you get the wrong answer either way (remsi gives 0 because the modulus is interpreted as -1, remui gives the wrong answer because the input is interpreted as a positive value mod 2**31).
I also think that this dialect is low-level enough that "preferences" about which coset representative is used are irrelevant. The lowering decides, and if we all agree to use
I think specifying an explicit type in the textual IR will be safer, but I'm willing to give this a shot as-is.
There was a problem hiding this comment.
Oh, that's very annoying. :/ I guess supporting the "q=2^31-1 in 31-bit underlying type" would require some kind of type conversion before we lower to arith? That just seems way overkill, so I'll make it a hard error for both signless and signed then!
There was a problem hiding this comment.
I ended up having to make it "just" a warning for now because I realized (a) the ops only take SignlessIntegerLike so we couldn't test the "non-simple" add/sub cases with the hard error in place and (b) while I tried relaxing the type constraint to a quickly defined IntegerLike, I realized actually using unsigned types completely breaks the lowering to arith, which insists on SignlessIntegerLike. (Why though?)
Since we were already planning to discuss data represetations/signedness stuff in tomorrow's meeting, I'll just add this to the pile :)
There was a problem hiding this comment.
Signless is required, I knew that. I just meant we should only be constraining the modulus, and interpreting the input values as part of the semantics.
|
The reason is that the attribute is now typed, so it has the type as an
additional field.
…On Wed, Jun 12, 2024, 11:24 AM Alexander Viand ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/Dialect/ArithExt/IR/ArithExtOps.td
<#731 (comment)>:
> @@ -16,8 +16,9 @@ class ArithExt_Op<string mnemonic, list<Trait> traits = [Pure]> :
class ArithExt_BinaryOp<string mnemonic, list<Trait> traits = []> :
ArithExt_Op<mnemonic, traits # [SameOperandsAndResultType, Pure, ElementwiseMappable]>,
- Arguments<(ins SignlessIntegerLike:$lhs, SignlessIntegerLike:$rhs, I64Attr:$modulus)>,
+ Arguments<(ins SignlessIntegerLike:$lhs, SignlessIntegerLike:$rhs, APIntAttr:$modulus)>,
Interesting! I guess it does make sense that one can use TypeOrAttrDefs
directly as a constraint, though I haven't seen Builtin_IntegerAttr used
that way upstream yet.
I tried swapping ut APIntAttr for Builtin_IntegerAttr to see what
happens, but before I even got to see what that does to the textual
representation, I noticed that getModulus() now returns the Attr
directly, whereas with APIntAttr, the tablegen backend would have that
function name be a helper function that returns an APInt, and demote the
actual Attr getter to getModulusAttr(). I'd love to say I understand way,
but I'm at "MLIR works in mysterious ways" 🤷♂️
—
Reply to this email directly, view it on GitHub
<#731 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAS2PKUVGJM57P7OMPTR7MTZHCG6ZAVCNFSM6AAAAABI7KW5YOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMJTHAYTGOJYGE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
* allow any integer attribute for arith_ext modulus * add lowering of arith_ext.add/sub/mul to arith * add `arith_ext.mac` operation
AlexanderViand-Intel
force-pushed
the
arith_ext_improvements
branch
from
93962bd
to
d6c2a50
Compare
|
@AlexanderViand-Intel - should we merge this and then revisit the changes we discussed in the last WG meeting? (since that'll involve an overhaul of NTT operation types) |
I think that sounds like a good plan! I'll create a draft PR for the modular/poly/etc overhaul next week :) |
asraa
added
the
pull_ready
This does three things
I64Attrconstraint (replacing it withAPIntAttr) and adds a verifier to ensure that the modulus can actually fit into the underlying operand type.arith_ext.mac %lhs, %rhs, %accoperation that multiplies the two first operands and then adds the third.This not only saves one remainder operation compared to doing the operations separately, but is also a native operation in some RLWE accelerators, so it's useful to have a way to model it in HEIR
arith_ext.add/sub/mul/mac to arith. If the results of a modular operation can fit into the underlying operand type, it's just a straighforward operation + remainder, otherwise it'll createextuiandremuioperations to temporarily increase the bitwidth.