From 5628815bfe48e6ac4bd566b423cc24b54810a4dd Mon Sep 17 00:00:00 2001 From: kalsprite Date: Mon, 24 Aug 2026 19:46:47 -0700 Subject: [PATCH] mod -1 --- src/llvm_backend_expr.cpp | 53 ++++++++++-------- tests/internal/test_mod.odin | 105 +++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 24 deletions(-) create mode 100644 tests/internal/test_mod.odin diff --git a/src/llvm_backend_expr.cpp b/src/llvm_backend_expr.cpp index 74cb46ceb..4b37ebf39 100644 --- a/src/llvm_backend_expr.cpp +++ b/src/llvm_backend_expr.cpp @@ -322,6 +322,21 @@ gb_internal IntegerDivisionByZeroKind lb_check_for_integer_division_by_zero_beha } +// LLVM has srem(min(Integer_Type), -1) as UB and it raises an FP exception on a hardware +// divide, yet `x % -1` is 0 for every x; `x srem 1` is 0 too and cannot trap, so a runtime +// -1 divisor can be swapped for 1. Vectorizable. +gb_internal LLVMValueRef lb_srem_safe_divisor(lbProcedure *p, LLVMValueRef rhs) { + LLVMValueRef minus_one = LLVMConstAllOnes(LLVMTypeOf(rhs)); + // build 1 as neg(-1), this folds for both scalars and vectors + LLVMValueRef one = LLVMBuildNeg(p->builder, minus_one, ""); + if (LLVMIsAConstantInt(rhs)) { + return rhs == minus_one ? one : rhs; + } + LLVMValueRef is_minus_one = LLVMBuildICmp(p->builder, LLVMIntEQ, rhs, minus_one, ""); + return LLVMBuildSelect(p->builder, is_minus_one, one, rhs, ""); +} + + // implements %% (the remainder/floored mod operator) on signed integers; // this is branchless and vectorizable, so it also covers vectors gb_internal LLVMValueRef lb_emit_signed_floor_mod(lbProcedure *p, LLVMValueRef lhs, LLVMValueRef rhs) { @@ -329,24 +344,11 @@ gb_internal LLVMValueRef lb_emit_signed_floor_mod(lbProcedure *p, LLVMValueRef l // and works for arbitrary precision integers, but the add can wrap at finite precision // the Odin spec mandates min(Integer_Type) %% -1 must be 0, - // but LLVM has srem(min(Integer_Type), -1) as UB and results in FP exception; - // since x %% -1 == 0 for every x, a constant rhs = -1 can fold, - // and a runtime -1 can be swapped with 1 (x srem 1 is 0 for every x, no exceptions) - LLVMValueRef minus_one = LLVMConstAllOnes(LLVMTypeOf(rhs)); - LLVMValueRef safe_rhs = rhs; - if (LLVMIsAConstantInt(rhs)) { - if (rhs == minus_one) { - return LLVMConstNull(LLVMTypeOf(rhs)); // the entire %% op folds to 0 - } - } else { - // safe_rhs = (rhs == -1) ? 1 : rhs - // vectorizable construction, - // build 1 as neg(-1), this folds for both scalars and vectors - LLVMValueRef one = LLVMBuildNeg(p->builder, minus_one, ""); - LLVMValueRef is_minus_one = LLVMBuildICmp(p->builder, LLVMIntEQ, rhs, minus_one, ""); - safe_rhs = LLVMBuildSelect(p->builder, is_minus_one, one, rhs, ""); + // a constant rhs = -1 can fold the whole operation, a runtime one is handled by the swap + if (LLVMIsAConstantInt(rhs) && rhs == LLVMConstAllOnes(LLVMTypeOf(rhs))) { + return LLVMConstNull(LLVMTypeOf(rhs)); // the entire %% op folds to 0 } - LLVMValueRef r = LLVMBuildSRem(p->builder, lhs, safe_rhs, ""); + LLVMValueRef r = LLVMBuildSRem(p->builder, lhs, lb_srem_safe_divisor(p, rhs), ""); // srem truncs to 0, so r needs a +rhs correction when the operands signs differ (and r != 0) // so we implement // r = lhs % rhs @@ -498,9 +500,10 @@ gb_internal bool lb_try_direct_vector_arith(lbProcedure *p, TokenKind op, lbValu } break; case Token_Mod: - { - auto *call = is_type_unsigned(integral_type) ? LLVMBuildURem : LLVMBuildSRem; - z = call(p->builder, x, y, ""); + if (is_type_unsigned(integral_type)) { + z = LLVMBuildURem(p->builder, x, y, ""); + } else { + z = LLVMBuildSRem(p->builder, x, lb_srem_safe_divisor(p, y), ""); } break; case Token_ModMod: @@ -610,9 +613,10 @@ gb_internal bool lb_try_direct_vector_arith(lbProcedure *p, TokenKind op, lbValu } break; case Token_Mod: - { - auto *call = is_type_unsigned(integral_type) ? LLVMBuildURem : LLVMBuildSRem; - z = call(p->builder, x, y, ""); + if (is_type_unsigned(integral_type)) { + z = LLVMBuildURem(p->builder, x, y, ""); + } else { + z = LLVMBuildSRem(p->builder, x, lb_srem_safe_divisor(p, y), ""); } break; case Token_ModMod: @@ -1684,7 +1688,8 @@ gb_internal LLVMValueRef lb_integer_modulo(lbProcedure *p, LLVMValueRef lhs, LLV if (is_unsigned) { return LLVMBuildURem(p->builder, lhs, rhs, ""); } else { - return LLVMBuildSRem(p->builder, lhs, rhs, ""); + // min(Integer_Type) % -1 is 0, matching the constant folder, and must not trap + return LLVMBuildSRem(p->builder, lhs, lb_srem_safe_divisor(p, rhs), ""); } } }; diff --git a/tests/internal/test_mod.odin b/tests/internal/test_mod.odin new file mode 100644 index 000000000..55217831b --- /dev/null +++ b/tests/internal/test_mod.odin @@ -0,0 +1,105 @@ +package test_internal + +import "core:testing" + +// % operator (truncated remainder) +// remainder = x - y * trunc(x / y) + +@(private="file") +trunc_mod :: proc(x, y: $T) -> T { + return x - y*(x/y) +} + +// this seems to prevent folding at least at -o:minimal +@(private="file") +not_const :: #force_no_inline proc(v: $T) -> T { return v } + +@(test) +mod_i8_exhaustive :: proc(t: ^testing.T) { + for i in -128..=127 { + for j in -128..=127 { + if j == 0 { continue } + // min(T) % -1 == 0 is tested in mod_exception, + // the trunc_mod reference itself would trap here + if i == -128 && j == -1 { continue } + x, y := i8(i), i8(j) + got := x % y + want := trunc_mod(x, y) + testing.expectf(t, got == want, "%v %% %v == %v, want %v", x, y, got, want) + } + } +} + +@(test) +mod_exception :: proc(t: ^testing.T) { + // min(T) % -1 is 0, which is what the constant folder answers + #assert(min(i8) % i8(-1) == 0) + #assert(min(i16) % i16(-1) == 0) + #assert(min(i32) % i32(-1) == 0) + #assert(min(i64) % i64(-1) == 0) + #assert(min(i128) % i128(-1) == 0) + + check :: proc(t: ^testing.T, $T: typeid, loc := #caller_location) { + x, y := not_const(min(T)), not_const(T(-1)) + testing.expectf(t, x % y == 0, "min(%v) %% -1 (rt divisor) == %v, want 0", typeid_of(T), x % y, loc = loc) + testing.expectf(t, x % -1 == 0, "min(%v) %% -1 (const divisor) == %v, want 0", typeid_of(T), x % -1, loc = loc) + } + check(t, i8) + check(t, i16) + check(t, i32) + check(t, i64) + check(t, i128) +} + +@(test) +mod_exception_vec :: proc(t: ^testing.T) { + { + // [4]i32 emits `srem <4 x i32>` + x := not_const([4]i32{min(i32), 0, -7, 5}) + y := not_const([4]i32{-1, -1, -1, -1}) + testing.expect_value(t, x % y, [4]i32{0, 0, 0, 0}) + } + { + // [16]i32 emits scalar `srem i32`, which is the other call site + x, y: [16]i32 + for i in 0..<16 { + x[i] = i == 0 ? min(i32) : i32(i) - 8 + y[i] = -1 + } + testing.expect_value(t, not_const(x) % not_const(y), [16]i32{}) + } +} + +@(test) +mod_assign :: proc(t: ^testing.T) { + // %= must agree with % + { + x := not_const(min(i32)) + y := not_const(i32(-1)) + x %= y + testing.expect_value(t, x, 0) + } + { + x := not_const(i64(-17)) + y := not_const(i64(5)) + x %= y + testing.expect_value(t, x, -17 % 5) + } +} + +@(test) +mod_unsigned_unchanged :: proc(t: ^testing.T) { + // the guard is signed-only; unsigned max is all-ones and must stay a real divisor + { + x, y := not_const(max(u32)), not_const(max(u32)) + testing.expect_value(t, x % y, 0) + } + { + x, y := not_const(u32(7)), not_const(max(u32)) + testing.expect_value(t, x % y, 7) + } + { + x, y := not_const(u8(200)), not_const(u8(255)) + testing.expect_value(t, x % y, 200) + } +}