From e4528b8abb627a6efd39128eb7f4f28fb7354e7f Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Wed, 27 Mar 2024 13:18:25 -0700 Subject: [PATCH] Add infrastructure for distinguishing between signed and unsigned integer types. (#3822) Because we don't have any unsigned integer types yet, this is mostly a no-op change, except that we now format negative values in SemIR properly. --- toolchain/check/check.cpp | 6 ++--- toolchain/check/eval.cpp | 23 +++++++++++-------- .../testdata/array/fail_bound_negative.carbon | 2 +- .../check/testdata/builtins/int_add.carbon | 2 +- .../check/testdata/builtins/int_div.carbon | 6 ++--- .../check/testdata/builtins/int_mod.carbon | 6 ++--- .../check/testdata/builtins/int_mul.carbon | 2 +- .../check/testdata/builtins/int_negate.carbon | 6 ++--- .../check/testdata/builtins/int_sub.carbon | 4 ++-- toolchain/lower/handle.cpp | 2 ++ toolchain/sem_ir/formatter.cpp | 8 +++++++ toolchain/sem_ir/type.h | 5 ++++ 12 files changed, 45 insertions(+), 27 deletions(-) diff --git a/toolchain/check/check.cpp b/toolchain/check/check.cpp index c5f945ad142b..ce5577cd9707 100644 --- a/toolchain/check/check.cpp +++ b/toolchain/check/check.cpp @@ -83,10 +83,8 @@ class SemIRDiagnosticConverter : public DiagnosticConverter { return sem_ir_->StringifyType(*type_id); } if (auto* typed_int = llvm::any_cast(&arg)) { - // TODO: Once unsigned integers are supported, compute the signedness - // here. - constexpr bool IsUnsigned = false; - return llvm::APSInt(typed_int->value, IsUnsigned); + return llvm::APSInt(typed_int->value, + !sem_ir_->types().IsSignedInt(typed_int->type)); } return DiagnosticConverter::ConvertArg(arg); } diff --git a/toolchain/check/eval.cpp b/toolchain/check/eval.cpp index 7c2ed0e7e112..978f963bb2c8 100644 --- a/toolchain/check/eval.cpp +++ b/toolchain/check/eval.cpp @@ -304,7 +304,7 @@ static auto PerformBuiltinUnaryIntOp(Context& context, SemIRLocation loc, auto op = context.insts().GetAs(arg_id); auto op_val = context.ints().Get(op.int_id); - if (op_val.isMinSignedValue()) { + if (context.types().IsSignedInt(op.type_id) && op_val.isMinSignedValue()) { CARBON_DIAGNOSTIC(CompileTimeIntegerNegateOverflow, Error, "Integer overflow in negation of {0}.", TypedInt); context.emitter().Emit(loc, CompileTimeIntegerNegateOverflow, @@ -328,20 +328,24 @@ static auto PerformBuiltinBinaryIntOp(Context& context, SemIRLocation loc, auto lhs_val = context.ints().Get(lhs.int_id); auto rhs_val = context.ints().Get(rhs.int_id); + bool is_signed = context.types().IsSignedInt(lhs.type_id); bool overflow = false; llvm::APInt result_val; llvm::StringLiteral op_str = ""; switch (builtin_kind) { case SemIR::BuiltinFunctionKind::IntAdd: - result_val = lhs_val.sadd_ov(rhs_val, overflow); + result_val = + is_signed ? lhs_val.sadd_ov(rhs_val, overflow) : lhs_val + rhs_val; op_str = "+"; break; case SemIR::BuiltinFunctionKind::IntSub: - result_val = lhs_val.ssub_ov(rhs_val, overflow); + result_val = + is_signed ? lhs_val.ssub_ov(rhs_val, overflow) : lhs_val - rhs_val; op_str = "-"; break; case SemIR::BuiltinFunctionKind::IntMul: - result_val = lhs_val.smul_ov(rhs_val, overflow); + result_val = + is_signed ? lhs_val.smul_ov(rhs_val, overflow) : lhs_val * rhs_val; op_str = "*"; break; case SemIR::BuiltinFunctionKind::IntDiv: @@ -349,7 +353,8 @@ static auto PerformBuiltinBinaryIntOp(Context& context, SemIRLocation loc, DiagnoseDivisionByZero(context, loc); return SemIR::ConstantId::Error; } - result_val = lhs_val.sdiv_ov(rhs_val, overflow); + result_val = is_signed ? lhs_val.sdiv_ov(rhs_val, overflow) + : lhs_val.udiv(rhs_val); op_str = "/"; break; case SemIR::BuiltinFunctionKind::IntMod: @@ -357,10 +362,10 @@ static auto PerformBuiltinBinaryIntOp(Context& context, SemIRLocation loc, DiagnoseDivisionByZero(context, loc); return SemIR::ConstantId::Error; } - result_val = lhs_val.srem(rhs_val); + result_val = is_signed ? lhs_val.srem(rhs_val) : lhs_val.urem(rhs_val); // LLVM weirdly lacks `srem_ov`, so we work it out for ourselves: // % -1 overflows because / -1 overflows. - overflow = (lhs_val.isMinSignedValue() && rhs_val.isAllOnes()); + overflow = is_signed && lhs_val.isMinSignedValue() && rhs_val.isAllOnes(); op_str = "%"; break; @@ -520,8 +525,8 @@ auto TryEvalInst(Context& context, SemIR::InstId inst_id, SemIR::Inst inst) // fits in 64 bits, not just that the bound does. Should we use a // 32-bit limit for 32-bit targets? const auto& bound_val = context.ints().Get(int_bound->int_id); - if (bound_val.isNegative()) { - // TODO: Skip this test if the bound type is unsigned. + if (context.types().IsSignedInt(int_bound->type_id) && + bound_val.isNegative()) { CARBON_DIAGNOSTIC(ArrayBoundNegative, Error, "Array bound of {0} is negative.", TypedInt); context.emitter().Emit(bound_id, ArrayBoundNegative, diff --git a/toolchain/check/testdata/array/fail_bound_negative.carbon b/toolchain/check/testdata/array/fail_bound_negative.carbon index 04825ab0877b..e6b81bd08fb7 100644 --- a/toolchain/check/testdata/array/fail_bound_negative.carbon +++ b/toolchain/check/testdata/array/fail_bound_negative.carbon @@ -15,7 +15,7 @@ var a: [i32; Negate(1)]; // CHECK:STDOUT: // CHECK:STDOUT: constants { // CHECK:STDOUT: %.1: i32 = int_literal 1 [template] -// CHECK:STDOUT: %.2: i32 = int_literal 4294967295 [template] +// CHECK:STDOUT: %.2: i32 = int_literal -1 [template] // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: file { diff --git a/toolchain/check/testdata/builtins/int_add.carbon b/toolchain/check/testdata/builtins/int_add.carbon index 3518b83965d3..01fa24033867 100644 --- a/toolchain/check/testdata/builtins/int_add.carbon +++ b/toolchain/check/testdata/builtins/int_add.carbon @@ -289,7 +289,7 @@ let b: i32 = Add(0x7FFFFFFF, 1); // CHECK:STDOUT: %.1: i32 = int_literal 2147483647 [template] // CHECK:STDOUT: %.2: i32 = int_literal 0 [template] // CHECK:STDOUT: %.3: i32 = int_literal 1 [template] -// CHECK:STDOUT: %.4: i32 = int_literal 2147483648 [template] +// CHECK:STDOUT: %.4: i32 = int_literal -2147483648 [template] // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: file { diff --git a/toolchain/check/testdata/builtins/int_div.carbon b/toolchain/check/testdata/builtins/int_div.carbon index 4ec35921cd76..5e09aad8e80e 100644 --- a/toolchain/check/testdata/builtins/int_div.carbon +++ b/toolchain/check/testdata/builtins/int_div.carbon @@ -115,10 +115,10 @@ let b: i32 = Div(0, 0); // CHECK:STDOUT: // CHECK:STDOUT: constants { // CHECK:STDOUT: %.1: i32 = int_literal 2147483647 [template] -// CHECK:STDOUT: %.2: i32 = int_literal 2147483649 [template] +// CHECK:STDOUT: %.2: i32 = int_literal -2147483647 [template] // CHECK:STDOUT: %.3: i32 = int_literal 1 [template] -// CHECK:STDOUT: %.4: i32 = int_literal 4294967295 [template] -// CHECK:STDOUT: %.5: i32 = int_literal 2147483648 [template] +// CHECK:STDOUT: %.4: i32 = int_literal -1 [template] +// CHECK:STDOUT: %.5: i32 = int_literal -2147483648 [template] // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: file { diff --git a/toolchain/check/testdata/builtins/int_mod.carbon b/toolchain/check/testdata/builtins/int_mod.carbon index 23ea91e6400d..359534f0853d 100644 --- a/toolchain/check/testdata/builtins/int_mod.carbon +++ b/toolchain/check/testdata/builtins/int_mod.carbon @@ -118,11 +118,11 @@ let b: i32 = Mod(0, 0); // CHECK:STDOUT: // CHECK:STDOUT: constants { // CHECK:STDOUT: %.1: i32 = int_literal 2147483647 [template] -// CHECK:STDOUT: %.2: i32 = int_literal 2147483649 [template] +// CHECK:STDOUT: %.2: i32 = int_literal -2147483647 [template] // CHECK:STDOUT: %.3: i32 = int_literal 1 [template] -// CHECK:STDOUT: %.4: i32 = int_literal 4294967295 [template] +// CHECK:STDOUT: %.4: i32 = int_literal -1 [template] // CHECK:STDOUT: %.5: i32 = int_literal 0 [template] -// CHECK:STDOUT: %.6: i32 = int_literal 2147483648 [template] +// CHECK:STDOUT: %.6: i32 = int_literal -2147483648 [template] // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: file { diff --git a/toolchain/check/testdata/builtins/int_mul.carbon b/toolchain/check/testdata/builtins/int_mul.carbon index bf41a7b81f3a..5ce5713490f0 100644 --- a/toolchain/check/testdata/builtins/int_mul.carbon +++ b/toolchain/check/testdata/builtins/int_mul.carbon @@ -92,7 +92,7 @@ let b: i32 = Mul(0x8000, 0x10000); // CHECK:STDOUT: %.2: i32 = int_literal 65536 [template] // CHECK:STDOUT: %.3: i32 = int_literal 2147418112 [template] // CHECK:STDOUT: %.4: i32 = int_literal 32768 [template] -// CHECK:STDOUT: %.5: i32 = int_literal 2147483648 [template] +// CHECK:STDOUT: %.5: i32 = int_literal -2147483648 [template] // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: file { diff --git a/toolchain/check/testdata/builtins/int_negate.carbon b/toolchain/check/testdata/builtins/int_negate.carbon index cae22934e2fa..8628066f045b 100644 --- a/toolchain/check/testdata/builtins/int_negate.carbon +++ b/toolchain/check/testdata/builtins/int_negate.carbon @@ -116,7 +116,7 @@ let b: i32 = Negate(Sub(Negate(0x7FFFFFFF), 1)); // CHECK:STDOUT: // CHECK:STDOUT: constants { // CHECK:STDOUT: %.1: i32 = int_literal 1 [template] -// CHECK:STDOUT: %.2: i32 = int_literal 4294967295 [template] +// CHECK:STDOUT: %.2: i32 = int_literal -1 [template] // CHECK:STDOUT: %.3: type = array_type %.1, i32 [template] // CHECK:STDOUT: %.4: type = ptr_type [i32; 1] [template] // CHECK:STDOUT: } @@ -306,9 +306,9 @@ let b: i32 = Negate(Sub(Negate(0x7FFFFFFF), 1)); // CHECK:STDOUT: // CHECK:STDOUT: constants { // CHECK:STDOUT: %.1: i32 = int_literal 2147483647 [template] -// CHECK:STDOUT: %.2: i32 = int_literal 2147483649 [template] +// CHECK:STDOUT: %.2: i32 = int_literal -2147483647 [template] // CHECK:STDOUT: %.3: i32 = int_literal 1 [template] -// CHECK:STDOUT: %.4: i32 = int_literal 2147483648 [template] +// CHECK:STDOUT: %.4: i32 = int_literal -2147483648 [template] // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: file { diff --git a/toolchain/check/testdata/builtins/int_sub.carbon b/toolchain/check/testdata/builtins/int_sub.carbon index c64a8b0030f0..0a1bfc5fb9cf 100644 --- a/toolchain/check/testdata/builtins/int_sub.carbon +++ b/toolchain/check/testdata/builtins/int_sub.carbon @@ -91,9 +91,9 @@ let c: i32 = Sub(Sub(0, 0x7FFFFFFF), 2); // CHECK:STDOUT: constants { // CHECK:STDOUT: %.1: i32 = int_literal 0 [template] // CHECK:STDOUT: %.2: i32 = int_literal 2147483647 [template] -// CHECK:STDOUT: %.3: i32 = int_literal 2147483649 [template] +// CHECK:STDOUT: %.3: i32 = int_literal -2147483647 [template] // CHECK:STDOUT: %.4: i32 = int_literal 1 [template] -// CHECK:STDOUT: %.5: i32 = int_literal 2147483648 [template] +// CHECK:STDOUT: %.5: i32 = int_literal -2147483648 [template] // CHECK:STDOUT: %.6: i32 = int_literal 2 [template] // CHECK:STDOUT: } // CHECK:STDOUT: diff --git a/toolchain/lower/handle.cpp b/toolchain/lower/handle.cpp index 69c69a786f23..0229ab5d60f9 100644 --- a/toolchain/lower/handle.cpp +++ b/toolchain/lower/handle.cpp @@ -172,6 +172,8 @@ auto HandleBuiltin(FunctionContext& /*context*/, SemIR::InstId /*inst_id*/, static auto HandleBuiltinCall(FunctionContext& context, SemIR::InstId inst_id, SemIR::BuiltinFunctionKind builtin_kind, llvm::ArrayRef arg_ids) -> void { + // TODO: Consider setting this to true in the performance build mode if the + // result type is a signed integer type. constexpr bool SignedOverflowIsUB = false; switch (builtin_kind) { diff --git a/toolchain/sem_ir/formatter.cpp b/toolchain/sem_ir/formatter.cpp index 208906244c00..745207b354b5 100644 --- a/toolchain/sem_ir/formatter.cpp +++ b/toolchain/sem_ir/formatter.cpp @@ -1150,6 +1150,13 @@ class Formatter { FormatTrailingBlock(inst.decl_block_id); } + auto FormatInstructionRHS(IntLiteral inst) -> void { + out_ << " "; + sem_ir_.ints() + .Get(inst.int_id) + .print(out_, sem_ir_.types().IsSignedInt(inst.type_id)); + } + auto FormatInstructionRHS(ImportRefUnused inst) -> void { // Don't format the inst_id because it refers to a different IR. // TODO: Consider a better way to format the InstID from other IRs. @@ -1212,6 +1219,7 @@ class Formatter { auto FormatArg(ImportIRId id) -> void { out_ << id; } auto FormatArg(IntId id) -> void { + // We don't know the signedness to use here. Default to unsigned. sem_ir_.ints().Get(id).print(out_, /*isSigned=*/false); } diff --git a/toolchain/sem_ir/type.h b/toolchain/sem_ir/type.h index fcf963042b9e..ce0f8ce1563e 100644 --- a/toolchain/sem_ir/type.h +++ b/toolchain/sem_ir/type.h @@ -83,6 +83,11 @@ class TypeStore : public ValueStore { return GetValueRepr(type_id).kind != ValueRepr::Unknown; } + // Determines whether the given type is a signed integer type. + auto IsSignedInt(TypeId int_type_id) const -> bool { + return GetInstId(int_type_id) == InstId::BuiltinIntType; + } + private: InstStore* insts_; };