[Python] Use Python floor-modulo semantics for % on integers - #5166
[Python] Use Python floor-modulo semantics for % on integers#5166udsy19 wants to merge 1 commit into
Conversation
The Python AST bridge lowered `%` on integers to `arith.remui`, an unsigned remainder, so any expression with a negative operand silently produced a wrong result: inside a kernel `-1 % 3` evaluated to 0 instead of 2, `-5 % 3` to 2 instead of 1, and `5 % -3` to 5 instead of -1. Emit `arith.remsi` instead and correct the truncated remainder by adding the divisor when the remainder is non-zero and its sign differs from the divisor's. That gives the remainder the sign of the divisor, as Python does, and matches the `arith.floordivsi` already emitted for `//`, so that `a == (a // b) * b + a % b` holds again. The bridge only ever builds signless integer types for Python `int`, `bool`, and the `numpy.intN` annotations, all of which are signed, so no unsigned operand type is affected. Fixes NVIDIA#5159 Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com>
|
Two scoping questions for maintainers, kept out of the description so the issue link reads cleanly:
Note the C++ bridge is correct as-is: it dispatches on signedness, which matches C++'s truncating |
The Python AST bridge lowered
%on integers toarith.remui, an unsignedremainder, so any expression with a negative operand silently produced a wrong
result inside a
@cudaq.kernel:This also broke the identity
a == (a // b) * b + a % b, because//in thesame bridge already lowers to the correctly signed
arith.floordivsi.What this changes
%on integers now emitsarith.remsiplus the floor correction that Python's%requires:arith.remsialone would not be enough: it truncates towards zero, so it fixesthe negative-dividend cases but still disagrees with Python whenever the divisor
is negative (
5 % -3would give2, not-1), and it would leave the(a // b) * b + a % bidentity broken againstarith.floordivsi. Adding thedivisor when the remainder is non-zero and its sign differs from the divisor's
gives the remainder the sign of the divisor, which is exactly Python's rule.
The correction is branch-free and only costs a compare/xor/compare/and/add/select
around the existing division;
arithcanonicalization folds it away when bothoperands are known constants.
Notes:
integer types (
IntegerType.get_signless) for Pythonint,bool, and thenumpy.int8/16/32/64annotations — all signed. There is no unsigned integerannotation in
python/cudaq/kernel/utils.py, so nothing relied onremui.cudaq/lib/Frontend/nvqpp/ConvertExpr.cpp(BO_Rem) it already dispatches onsignedness —
arith::RemUIOpfor unsigned C++ integer types,arith::RemSIOpfor signed ones — which is the right lowering for C++'s truncating
%. Nochange is needed or wanted there; the two frontends now each match their own
source language instead of the Python one accidentally using an unsigned
lowering for signed types.
arith.remsiby zeroand
arith.remsi(INT_MIN, -1)are UB in LLVM, exactly as thearith.divsiunderlying the
arith.floordivsialready emitted for//is. So a kernelcomputing
a // bis already exposed to both today, and this change does notadd a new class of exposure — but it does move
%onto the signed divisioninstruction, so on targets where signed division traps (x86
idiv),a % bwithb == 0or(a, b) == (INT_MIN, -1)can now trap where the oldremuisilently returned a wrong value (INT_MIN % -1returnedINT_MIN;Python says
0). Guarding it was deliberately not done: it would cost everymodulo an extra compare and select, and would be inconsistent with the
unguarded
//right above it. Happy to guard both together if maintainersprefer.
arith.remsigives|r| < |b|, andthe
r + bis only selected whenr != 0andrandbhave oppositesigns, so the sum lies strictly between
0andband is alwaysrepresentable — including for
b == INT_MIN.%path is left alone in this PR.arith.remfalsotruncates towards zero, so
-1.0 % 3.0in a kernel returns-1.0wherePython returns
2.0. Matching CPython there needs the same correction plusits
copysign(0.0, divisor)handling of a zero remainder, and it has to bethought through for inf/NaN operands, so it deserves its own change. Happy to
follow up with it if maintainers want it in the same PR.
Testing
python/tests/kernel/test_kernel_float.py: addstest_integer_modulo_matches_python(the reported cases, forintandnp.int32) andtest_integer_floor_division_modulo_identity(checksa == (a // b) * b + a % bover a sweep of signed pairs), next to theexisting
test_integer_floor_division_matches_python.python/tests/mlir/{ast_break,ast_continue,ast_elif,ast_iterate_loop_init}.py:CHECK lines updated for the new
i % 4expansion. No new constant appears inthe entry block — the
0constant CSEs with the one already there, so theexisting
CHECK-DAGconstant block is untouched. The trailingVAL_*capturenames in each file are renumbered so they stay unique and increasing, matching
how these generated files are written; FileCheck itself would accept the old
numbers, but leaving them would make two different values share a name inside
one function's CHECK block.
Fixes #5159