From 1717d42a41fdcef4d8eb3afdf08e5f581c9ed137 Mon Sep 17 00:00:00 2001 From: Raphael Amorim Date: Thu, 14 May 2026 10:05:58 +0200 Subject: [PATCH] fix signed shr --- src/ast.cpp | 30 ++++++++++++--- src/jam_llvm.cpp | 6 +++ src/jam_llvm.h | 3 ++ src/parser.cpp | 39 +++++++++++++++++++ tests/unit/test_signed_shr.jam | 70 ++++++++++++++++++++++++++++++++++ 5 files changed, 143 insertions(+), 5 deletions(-) create mode 100644 tests/unit/test_signed_shr.jam diff --git a/src/ast.cpp b/src/ast.cpp index fa0d1f7..24d7fc2 100644 --- a/src/ast.cpp +++ b/src/ast.cpp @@ -927,8 +927,19 @@ static JamValueRef codegenBinaryOp(JamCodegenContext &ctx, const AstNode &n) { return JamLLVMBuildXor(ctx.getBuilder(), L, R, "xortmp"); case BinOp::Shl: return JamLLVMBuildShl(ctx.getBuilder(), L, R, "shltmp"); - case BinOp::Shr: - return JamLLVMBuildLShr(ctx.getBuilder(), L, R, "shrtmp"); + case BinOp::Shr: { + // this one was a pain because: + // arithmetic shift right for signed operands so the sign bit + // propagates (matches C's behaviour for signed >>); logical + // shift for unsigned. Without this an `i64` value of -16 >> 12 + // became a huge positive number, which broke the psone emulator + // IR-clamp pipeline and made every 3D scene render as a + // stretched streak. + bool signed_ = + isSignedIntExpr(ctx, lhsIdx) || isSignedIntExpr(ctx, rhsIdx); + return signed_ ? JamLLVMBuildAShr(ctx.getBuilder(), L, R, "shrtmp") + : JamLLVMBuildLShr(ctx.getBuilder(), L, R, "shrtmp"); + } case BinOp::Eq: return JamLLVMBuildICmp(ctx.getBuilder(), JAM_ICMP_EQ, L, R, "cmptmp"); case BinOp::Ne: @@ -3045,7 +3056,13 @@ JamValueRef codegenNode(JamCodegenContext &ctx, NodeIdx node, NodeIdx operandIdx = static_cast(n.lhs); TypeIdx targetTy = static_cast(n.rhs); JamTypeRef targetLLVM = ctx.getLLVMType(targetTy); - JamValueRef val = codegenNode(ctx, operandIdx); + const AstNode &operandNode = ctx.getNodeStore().get(operandIdx); + JamTypeRef innerExpected = + (operandNode.tag == AstTag::NumberLit && + JamLLVMTypeIsInteger(targetLLVM)) + ? targetLLVM + : nullptr; + JamValueRef val = codegenNode(ctx, operandIdx, innerExpected); if (!val) return nullptr; JamTypeRef srcLLVM = JamLLVMTypeOf(val); if (srcLLVM == targetLLVM) return val; @@ -3061,8 +3078,11 @@ JamValueRef codegenNode(JamCodegenContext &ctx, NodeIdx node, "as.tag.cast"); } if (JamLLVMTypeIsInteger(srcLLVM) && JamLLVMTypeIsInteger(targetLLVM)) { - return JamLLVMBuildIntCast(ctx.getBuilder(), val, targetLLVM, false, - "as.icast"); + // sign-extend when widening a signed source so e.g. an i8 + // holding -16 becomes i64 -16 rather than i64 +240. + bool signedSrc = isSignedIntExpr(ctx, operandIdx); + return JamLLVMBuildIntCast(ctx.getBuilder(), val, targetLLVM, + signedSrc, "as.icast"); } if (JamLLVMTypeIsInteger(srcLLVM) && JamLLVMTypeIsFloat(targetLLVM)) { return JamLLVMBuildSIToFP(ctx.getBuilder(), val, targetLLVM, diff --git a/src/jam_llvm.cpp b/src/jam_llvm.cpp index 94dabde..e186e70 100644 --- a/src/jam_llvm.cpp +++ b/src/jam_llvm.cpp @@ -641,6 +641,12 @@ JamValueRef JamLLVMBuildLShr(JamBuilderRef builder, JamValueRef lhs, UNWRAP_VALUE(lhs), UNWRAP_VALUE(rhs), name)); } +JamValueRef JamLLVMBuildAShr(JamBuilderRef builder, JamValueRef lhs, + JamValueRef rhs, const char *name) { + return WRAP_VALUE(UNWRAP_BUILDER(builder)->CreateAShr( + UNWRAP_VALUE(lhs), UNWRAP_VALUE(rhs), name)); +} + JamValueRef JamLLVMBuildICmp(JamBuilderRef builder, JamIntPredicate pred, JamValueRef lhs, JamValueRef rhs, const char *name) { diff --git a/src/jam_llvm.h b/src/jam_llvm.h index 9024d7e..fa5b428 100644 --- a/src/jam_llvm.h +++ b/src/jam_llvm.h @@ -255,6 +255,9 @@ JAM_EXTERN_C JamValueRef JamLLVMBuildShl(JamBuilderRef builder, JamValueRef lhs, JAM_EXTERN_C JamValueRef JamLLVMBuildLShr(JamBuilderRef builder, JamValueRef lhs, JamValueRef rhs, const char *name); +JAM_EXTERN_C JamValueRef JamLLVMBuildAShr(JamBuilderRef builder, + JamValueRef lhs, JamValueRef rhs, + const char *name); JAM_EXTERN_C JamValueRef JamLLVMBuildICmp(JamBuilderRef builder, JamIntPredicate pred, JamValueRef lhs, diff --git a/src/parser.cpp b/src/parser.cpp index d649ed3..1c9a5aa 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -968,6 +968,45 @@ NodeIdx Parser::parseUnary() { } if (match(TOK_MINUS)) { NodeIdx operand = parseUnary(); + // fold a leading "-" into the literal's sign so the natural type + // is signed. Otherwise "-16 as i64" parses as "Neg(AsCast(16, + // i64))" where the inner 16 is a u8. + auto innerLit = [&](NodeIdx idx) -> NodeIdx { + NodeIdx cur = idx; + while (true) { + const AstNode &n = nodes->get(cur); + if (n.tag == AstTag::NumberLit) { return cur; } + if (n.tag == AstTag::AsCast) { + cur = static_cast(n.lhs); + continue; + } + return kNoNode; + } + }; + NodeIdx litIdx = innerLit(operand); + if (litIdx != kNoNode) { + AstNode lit = nodes->get(litIdx); + lit.flags = static_cast(lit.flags ^ 1u); + NodeIdx newLit = emit(lit); + // if the original operand was a cast, rebuild the cast chain + // around the sign-flipped literal so "-16 as i64" becomes + // "AsCast(NumberLit(-16), i64)". + if (operand != litIdx) { + NodeIdx cur = operand; + std::vector casts; + while (cur != litIdx) { + const AstNode &n = nodes->get(cur); + casts.push_back(static_cast(n.rhs)); + cur = static_cast(n.lhs); + } + for (auto it = casts.rbegin(); it != casts.rend(); ++it) { + newLit = emit(AstNode{AstTag::AsCast, 0, 0, 0, + static_cast(newLit), + static_cast(*it)}); + } + } + return wrapAs(newLit); + } return wrapAs( emit(AstNode{AstTag::UnaryOp, static_cast(UnaryOp::Neg), 0, 0, static_cast(operand), 0})); diff --git a/tests/unit/test_signed_shr.jam b/tests/unit/test_signed_shr.jam new file mode 100644 index 0000000..084e497 --- /dev/null +++ b/tests/unit/test_signed_shr.jam @@ -0,0 +1,70 @@ +// Signed-arithmetic vs unsigned-logical right-shift dispatch. + +const { assert } = import("test"); + +fn shrI64(a: i64, b: i64) i64 { return a >> b; } +fn shrI32(a: i32, b: i32) i32 { return a >> b; } +fn shrI16(a: i16, b: i16) i16 { return a >> b; } +fn shrI8(a: i8, b: i8) i8 { return a >> b; } +fn shrU64(a: u64, b: u64) u64 { return a >> b; } +fn shrU32(a: u32, b: u32) u32 { return a >> b; } + +// Signed shifts must preserve the sign bit (arithmetic shift right) + +tfn shrI64Negative() { + // -16 >> 12 = -1 + assert(shrI64(-16 as i64, 12 as i64) as i32, -1); + // -61843 >> 12 = -16 + assert(shrI64(-61843 as i64, 12 as i64) as i32, -16); + // -1 >> 4 = -1 + assert(shrI64(-1 as i64, 4 as i64) as i32, -1); + // -256 >> 4 = -16 + assert(shrI64(-256 as i64, 4 as i64) as i32, -16); +} + +tfn shrI64Positive() { + // Positive values are unchanged by the AShr/LShr distinction + assert(shrI64(65536 as i64, 16 as i64) as i32, 1); + assert(shrI64(1024 as i64, 4 as i64) as i32, 64); + assert(shrI64(0 as i64, 12 as i64) as i32, 0); +} + +tfn shrI32Negative() { + // -16 >> 12 = -1 + assert(shrI32(-16, 12), -1); + // INT32_MIN >> 31 = -1 (sign bit propagates all the way) + assert(shrI32(-2147483648, 31), -1); +} + +tfn shrI16Negative() { + // -256 >> 4 = -16 + assert(shrI16(-256 as i16, 4 as i16) as i32, -16); + // -32768 >> 15 = -1 + assert(shrI16(-32768 as i16, 15 as i16) as i32, -1); +} + +tfn shrI8Negative() { + // -128 >> 7 = -1 + assert(shrI8(-128 as i8, 7 as i8) as i32, -1); + // -16 >> 2 = -4 + assert(shrI8(-16 as i8, 2 as i8) as i32, -4); +} + +// Unsigned shifts must NOT propagate any sign bit (logical shift right). + +tfn shrU32Top() { + // 0xFFFFFFFF >> 1 = 0x7FFFFFFF + assert(shrU32(4294967295, 1), 2147483647); + // 0x80000000 >> 31 = 1 (not -1) + assert(shrU32(2147483648, 31), 1); +} + +tfn shrU64Top() { + // 0xFFFFFFFFFFFFFFFF >> 1 should NOT propagate the high bit. + // u64 max as i32 truncation gives -1; (u64 max >> 1) as i32 gives -1 + // too, but the upper-half check separates the two: shifted >> 32 + // must be 0x7FFFFFFF for logical, 0xFFFFFFFF (-1) for arithmetic. + const v: u64 = shrU64(18446744073709551615, 1); + const hi: u32 = (v >> 32) as u32; + assert(hi as i32, 2147483647); // = 0x7FFFFFFF +} -- 2.51.2