Skip to content

Commit

Permalink
Fix lowering of a conversion from a type with a pointer value represe…
Browse files Browse the repository at this point in the history
…ntation 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.
  • Loading branch information
zygoloid authored Nov 1, 2024
1 parent fab0772 commit 32e5212
Show file tree
Hide file tree
Showing 5 changed files with 95 additions and 7 deletions.
8 changes: 4 additions & 4 deletions toolchain/check/convert.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<SemIR::BindValue>(
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]];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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: }
Expand Down
6 changes: 6 additions & 0 deletions toolchain/lower/constant.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) \
Expand Down
3 changes: 2 additions & 1 deletion toolchain/lower/file_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}

Expand Down
81 changes: 81 additions & 0 deletions toolchain/lower/testdata/class/convert.carbon
Original file line number Diff line number Diff line change
@@ -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)

0 comments on commit 32e5212

Please sign in to comment.