feat(field): add PowPFOp to support prime field exponent #67
feat(field): add PowPFOp to support prime field exponent #67
PowPFOp to support prime field exponent #67Conversation
34f54c0 to
cdcc259
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces PowPFOp to support exponentiation where the exponent is a prime field element. This is a great feature that enhances the flexibility of power operations. The implementation refactors the existing PowUIOp logic into a shared computePower helper function, which is a clean approach to avoid code duplication. The new functionality is also well-tested. I have one suggestion to improve the readability of the computePower function for better long-term maintainability.
| modulus = modulus.zext(modBitWidth * 2); | ||
| modulus = modulus * modulus - 1; | ||
| exp = b.create<arith::ExtUIOp>( | ||
| IntegerType::get(exp.getContext(), modulus.getBitWidth()), exp); | ||
| intType = IntegerType::get(exp.getContext(), modulus.getBitWidth()); | ||
| exp = b.create<arith::RemUIOp>( | ||
| exp, b.create<arith::ConstantIntOp>(intType, modulus)); |
There was a problem hiding this comment.
The modulus variable is reused to store the group order (p^2 - 1), which can be confusing as it originally holds the prime modulus p. Using a separate, more descriptively named variable like groupOrder would make the logic for exponent reduction clearer and improve maintainability.
| modulus = modulus.zext(modBitWidth * 2); | |
| modulus = modulus * modulus - 1; | |
| exp = b.create<arith::ExtUIOp>( | |
| IntegerType::get(exp.getContext(), modulus.getBitWidth()), exp); | |
| intType = IntegerType::get(exp.getContext(), modulus.getBitWidth()); | |
| exp = b.create<arith::RemUIOp>( | |
| exp, b.create<arith::ConstantIntOp>(intType, modulus)); | |
| APInt groupOrder = modulus.zext(modBitWidth * 2); | |
| groupOrder = groupOrder * groupOrder - 1; | |
| exp = b.create<arith::ExtUIOp>( | |
| IntegerType::get(exp.getContext(), groupOrder.getBitWidth()), exp); | |
| intType = IntegerType::get(exp.getContext(), groupOrder.getBitWidth()); | |
| exp = b.create<arith::RemUIOp>( | |
| exp, b.create<arith::ConstantIntOp>(intType, groupOrder)); |
|
chore(feat): fix description: the scope should be field |
Description
This PR introduces
PowPFOpwhich can take prime field element as an exponent so that HLO power operation can handle both scalar field type and primitive unsigned integer type as an exponent.Related Issues/PRs
PowOp#61Checklist