From 32e5212daaff256ffb951b501dd4a552a2c1cbf2 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Fri, 1 Nov 2024 07:52:57 -0700 Subject: [PATCH] Fix lowering of a conversion from a type with a pointer value representation to a type with a copy value representation. (#4467) We previously generated a `value_bind` instruction of the wrong type, resulting in lowering building bad LLVM IR. Also fix another issue exposed by this change, where we would import constants without marking their types as complete, and then crash in lowering while trying to lower them. This happens in particular for the `FunctionType`s of functions in `ImplDecl`s. Address this by skipping lowering for constants with incomplete types. --- toolchain/check/convert.cpp | 8 +- .../operators/overloaded/implicit_as.carbon | 4 +- toolchain/lower/constant.cpp | 6 ++ toolchain/lower/file_context.h | 3 +- toolchain/lower/testdata/class/convert.carbon | 81 +++++++++++++++++++ 5 files changed, 95 insertions(+), 7 deletions(-) create mode 100644 toolchain/lower/testdata/class/convert.carbon diff --git a/toolchain/check/convert.cpp b/toolchain/check/convert.cpp index 4c4f0e90bc33..8b5b265d14d8 100644 --- a/toolchain/check/convert.cpp +++ b/toolchain/check/convert.cpp @@ -975,8 +975,7 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id, } // If this is not a builtin conversion, try an `ImplicitAs` conversion. - SemIR::Inst expr = sem_ir.insts().Get(expr_id); - if (expr.type_id() != target.type_id) { + if (sem_ir.insts().Get(expr_id).type_id() != target.type_id) { SemIR::InstId interface_args[] = { context.types().GetInstId(target.type_id)}; Operator op = { @@ -1019,7 +1018,8 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id, switch (SemIR::GetExprCategory(sem_ir, expr_id)) { case SemIR::ExprCategory::NotExpr: case SemIR::ExprCategory::Mixed: - CARBON_FATAL("Unexpected expression {0} after builtin conversions", expr); + CARBON_FATAL("Unexpected expression {0} after builtin conversions", + sem_ir.insts().Get(expr_id)); case SemIR::ExprCategory::Error: return SemIR::InstId::BuiltinError; @@ -1057,7 +1057,7 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id, // TODO: Support types with custom value representations. expr_id = context.AddInst( context.insts().GetLocId(expr_id), - {.type_id = expr.type_id(), .value_id = expr_id}); + {.type_id = target.type_id, .value_id = expr_id}); // We now have a value expression. [[fallthrough]]; diff --git a/toolchain/check/testdata/operators/overloaded/implicit_as.carbon b/toolchain/check/testdata/operators/overloaded/implicit_as.carbon index bf116f85f13d..9995aefe5b5a 100644 --- a/toolchain/check/testdata/operators/overloaded/implicit_as.carbon +++ b/toolchain/check/testdata/operators/overloaded/implicit_as.carbon @@ -313,7 +313,7 @@ fn Test() { // CHECK:STDOUT: %.loc30_18.7: init i32 = converted %Source.call.loc30, %Convert.call.loc30 // CHECK:STDOUT: %.loc30_18.8: ref i32 = temporary_storage // CHECK:STDOUT: %.loc30_18.9: ref i32 = temporary %.loc30_18.8, %.loc30_18.7 -// CHECK:STDOUT: %.loc30_18.10: %X = bind_value %.loc30_18.9 +// CHECK:STDOUT: %.loc30_18.10: i32 = bind_value %.loc30_18.9 // CHECK:STDOUT: %Sink_i32.call: init %.1 = call %Sink_i32.ref(%.loc30_18.10) // CHECK:STDOUT: %Sink_X.ref: %Sink_X.type = name_ref Sink_X, file.%Sink_X.decl [template = constants.%Sink_X] // CHECK:STDOUT: %Source.ref.loc31: %Source.type = name_ref Source, file.%Source.decl [template = constants.%Source] @@ -334,7 +334,7 @@ fn Test() { // CHECK:STDOUT: %Convert.call.loc31: init %X = call %.loc31_16.7(%.loc31_16.9) to %.loc31_16.8 // CHECK:STDOUT: %.loc31_16.10: init %X = converted %Source.call.loc31, %Convert.call.loc31 // CHECK:STDOUT: %.loc31_16.11: ref %X = temporary %.loc31_16.8, %.loc31_16.10 -// CHECK:STDOUT: %.loc31_16.12: i32 = bind_value %.loc31_16.11 +// CHECK:STDOUT: %.loc31_16.12: %X = bind_value %.loc31_16.11 // CHECK:STDOUT: %Sink_X.call: init %.1 = call %Sink_X.ref(%.loc31_16.12) // CHECK:STDOUT: return // CHECK:STDOUT: } diff --git a/toolchain/lower/constant.cpp b/toolchain/lower/constant.cpp index 0d93f8aab91c..1f6021fbf10e 100644 --- a/toolchain/lower/constant.cpp +++ b/toolchain/lower/constant.cpp @@ -260,6 +260,12 @@ auto LowerConstants(FileContext& file_context, } auto inst = file_context.sem_ir().insts().Get(inst_id); + if (inst.type_id().is_valid() && + !file_context.sem_ir().types().IsComplete(inst.type_id())) { + // If a constant doesn't have a complete type, that means we imported it + // but didn't actually use it. + continue; + } llvm::Constant* value = nullptr; CARBON_KIND_SWITCH(inst) { #define CARBON_SEM_IR_INST_KIND(Name) \ diff --git a/toolchain/lower/file_context.h b/toolchain/lower/file_context.h index 0c087d8f7337..c2f1d5772249 100644 --- a/toolchain/lower/file_context.h +++ b/toolchain/lower/file_context.h @@ -55,7 +55,8 @@ class FileContext { auto GetType(SemIR::TypeId type_id) -> llvm::Type* { // InvalidType should not be passed in. CARBON_CHECK(type_id.index >= 0, "{0}", type_id); - CARBON_CHECK(types_[type_id.index], "Missing type {0}", type_id); + CARBON_CHECK(types_[type_id.index], "Missing type {0}: {1}", type_id, + sem_ir().types().GetAsInst(type_id)); return types_[type_id.index]; } diff --git a/toolchain/lower/testdata/class/convert.carbon b/toolchain/lower/testdata/class/convert.carbon new file mode 100644 index 000000000000..4c9fab477eb7 --- /dev/null +++ b/toolchain/lower/testdata/class/convert.carbon @@ -0,0 +1,81 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +// AUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/lower/testdata/class/convert.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/lower/testdata/class/convert.carbon + +class IntWrapper { + var n: i32; +} + +impl IntWrapper as Core.ImplicitAs(i32) { + fn Convert[self: Self]() -> i32 { return self.n; } +} + +fn Consume(n: i32) {} + +fn DoIt() { + var w: IntWrapper = {.n = 42}; + Consume(w); +} + +// CHECK:STDOUT: ; ModuleID = 'convert.carbon' +// CHECK:STDOUT: source_filename = "convert.carbon" +// CHECK:STDOUT: +// CHECK:STDOUT: @struct.loc22_32 = internal constant { i32 } { i32 42 } +// CHECK:STDOUT: +// CHECK:STDOUT: define i32 @"_CConvert.IntWrapper.Main:ImplicitAs.Core"(ptr %self) !dbg !4 { +// CHECK:STDOUT: entry: +// CHECK:STDOUT: %.loc16_48.1.n = getelementptr inbounds nuw { i32 }, ptr %self, i32 0, i32 0, !dbg !7 +// CHECK:STDOUT: %.loc16_48.2 = load i32, ptr %.loc16_48.1.n, align 4, !dbg !7 +// CHECK:STDOUT: ret i32 %.loc16_48.2, !dbg !8 +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: define void @_CConsume.Main(i32 %n) !dbg !9 { +// CHECK:STDOUT: entry: +// CHECK:STDOUT: ret void, !dbg !10 +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: define void @_CDoIt.Main() !dbg !11 { +// CHECK:STDOUT: entry: +// CHECK:STDOUT: %w.var = alloca { i32 }, align 8, !dbg !12 +// CHECK:STDOUT: %.loc22_31.2.n = getelementptr inbounds nuw { i32 }, ptr %w.var, i32 0, i32 0, !dbg !13 +// CHECK:STDOUT: call void @llvm.memcpy.p0.p0.i64(ptr align 4 %w.var, ptr align 4 @struct.loc22_32, i64 4, i1 false), !dbg !14 +// CHECK:STDOUT: %Convert.call = call i32 @"_CConvert.IntWrapper.Main:ImplicitAs.Core"(ptr %w.var), !dbg !15 +// CHECK:STDOUT: %.loc23_11.6.temp = alloca i32, align 4, !dbg !15 +// CHECK:STDOUT: store i32 %Convert.call, ptr %.loc23_11.6.temp, align 4, !dbg !15 +// CHECK:STDOUT: %.loc23_11.8 = load i32, ptr %.loc23_11.6.temp, align 4, !dbg !15 +// CHECK:STDOUT: call void @_CConsume.Main(i32 %.loc23_11.8), !dbg !16 +// CHECK:STDOUT: ret void, !dbg !17 +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: ; Function Attrs: nocallback nofree nounwind willreturn memory(argmem: readwrite) +// CHECK:STDOUT: declare void @llvm.memcpy.p0.p0.i64(ptr noalias nocapture writeonly, ptr noalias nocapture readonly, i64, i1 immarg) #0 +// CHECK:STDOUT: +// CHECK:STDOUT: attributes #0 = { nocallback nofree nounwind willreturn memory(argmem: readwrite) } +// CHECK:STDOUT: +// CHECK:STDOUT: !llvm.module.flags = !{!0, !1} +// CHECK:STDOUT: !llvm.dbg.cu = !{!2} +// CHECK:STDOUT: +// CHECK:STDOUT: !0 = !{i32 7, !"Dwarf Version", i32 5} +// CHECK:STDOUT: !1 = !{i32 2, !"Debug Info Version", i32 3} +// CHECK:STDOUT: !2 = distinct !DICompileUnit(language: DW_LANG_C, file: !3, producer: "carbon", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug) +// CHECK:STDOUT: !3 = !DIFile(filename: "convert.carbon", directory: "") +// CHECK:STDOUT: !4 = distinct !DISubprogram(name: "Convert", linkageName: "_CConvert.IntWrapper.Main:ImplicitAs.Core", scope: null, file: !3, line: 16, type: !5, spFlags: DISPFlagDefinition, unit: !2) +// CHECK:STDOUT: !5 = !DISubroutineType(types: !6) +// CHECK:STDOUT: !6 = !{} +// CHECK:STDOUT: !7 = !DILocation(line: 16, column: 44, scope: !4) +// CHECK:STDOUT: !8 = !DILocation(line: 16, column: 37, scope: !4) +// CHECK:STDOUT: !9 = distinct !DISubprogram(name: "Consume", linkageName: "_CConsume.Main", scope: null, file: !3, line: 19, type: !5, spFlags: DISPFlagDefinition, unit: !2) +// CHECK:STDOUT: !10 = !DILocation(line: 19, column: 1, scope: !9) +// CHECK:STDOUT: !11 = distinct !DISubprogram(name: "DoIt", linkageName: "_CDoIt.Main", scope: null, file: !3, line: 21, type: !5, spFlags: DISPFlagDefinition, unit: !2) +// CHECK:STDOUT: !12 = !DILocation(line: 22, column: 7, scope: !11) +// CHECK:STDOUT: !13 = !DILocation(line: 22, column: 23, scope: !11) +// CHECK:STDOUT: !14 = !DILocation(line: 22, column: 3, scope: !11) +// CHECK:STDOUT: !15 = !DILocation(line: 23, column: 11, scope: !11) +// CHECK:STDOUT: !16 = !DILocation(line: 23, column: 3, scope: !11) +// CHECK:STDOUT: !17 = !DILocation(line: 21, column: 1, scope: !11)