From 35c2142392844a1ca03f4d60c68df8e0fbb6e773 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Thu, 2 Nov 2023 09:49:23 -0700 Subject: [PATCH] Treat field access into a class value expression as a value expression. (#3357) Per the design, field access into a class value expression is a value expression, even though we could produce an ephemeral reference expression instead and avoid performing a value binding. This slightly pessimizes class member access in some cases, but we should be able to restore the old generated code by deferring actually performing the value binding until a value expression is needed. --- toolchain/check/handle_name.cpp | 19 ++++- .../class/fail_memaccess_category.carbon | 81 +++++++++++++++++++ .../class/field_access_in_value.carbon | 2 +- .../lower/testdata/class/value_access.carbon | 22 ++++- 4 files changed, 116 insertions(+), 8 deletions(-) create mode 100644 toolchain/check/testdata/class/fail_memaccess_category.carbon diff --git a/toolchain/check/handle_name.cpp b/toolchain/check/handle_name.cpp index cc01e91ec093..b6c565cb1bcd 100644 --- a/toolchain/check/handle_name.cpp +++ b/toolchain/check/handle_name.cpp @@ -125,10 +125,21 @@ auto HandleMemberAccessExpression(Context& context, Parse::Node parse_node) CARBON_CHECK(field) << "Unexpected value " << context.nodes().Get(field_id) << " for field name expression"; - context.AddNodeAndPush( - parse_node, SemIR::ClassFieldAccess{ - parse_node, unbound_field_type->field_type_id, - base_id, field->index}); + auto access_id = context.AddNode(SemIR::ClassFieldAccess{ + parse_node, unbound_field_type->field_type_id, base_id, + field->index}); + if (SemIR::GetExpressionCategory(context.sem_ir(), base_id) == + SemIR::ExpressionCategory::Value && + SemIR::GetExpressionCategory(context.sem_ir(), access_id) != + SemIR::ExpressionCategory::Value) { + // Class field access on a value expression produces an ephemeral + // reference if the class's value representation is a pointer to the + // object representation. Add a value binding in that case so that the + // expression category of the result matches the expression category + // of the base. + access_id = ConvertToValueExpression(context, access_id); + } + context.node_stack().Push(parse_node, access_id); return true; } if (member_type_id == diff --git a/toolchain/check/testdata/class/fail_memaccess_category.carbon b/toolchain/check/testdata/class/fail_memaccess_category.carbon new file mode 100644 index 000000000000..e17899540755 --- /dev/null +++ b/toolchain/check/testdata/class/fail_memaccess_category.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 + +class A { + fn F[addr self: A*](); +} + +class B { + var a: A; +} + +fn F(s: {.a: A}, b: B) { + // `s` has only a value representation, so this must be invalid. + // CHECK:STDERR: fail_memaccess_category.carbon:[[@LINE+6]]:8: ERROR: `addr self` method cannot be invoked on a value. + // CHECK:STDERR: s.a.F(); + // CHECK:STDERR: ^ + // CHECK:STDERR: fail_memaccess_category.carbon:[[@LINE-12]]:13: Initializing `addr self` parameter of method declared here. + // CHECK:STDERR: fn F[addr self: A*](); + // CHECK:STDERR: ^ + s.a.F(); + + // `b` has an object representation for `A`, but this is still invalid for + // consistency. + // CHECK:STDERR: fail_memaccess_category.carbon:[[@LINE+6]]:8: ERROR: `addr self` method cannot be invoked on a value. + // CHECK:STDERR: b.a.F(); + // CHECK:STDERR: ^ + // CHECK:STDERR: fail_memaccess_category.carbon:[[@LINE-22]]:13: Initializing `addr self` parameter of method declared here. + // CHECK:STDERR: fn F[addr self: A*](); + // CHECK:STDERR: ^ + b.a.F(); +} + +// CHECK:STDOUT: file "fail_memaccess_category.carbon" { +// CHECK:STDOUT: class_declaration @A, () +// CHECK:STDOUT: %A: type = class_type @A +// CHECK:STDOUT: %.loc9: type = struct_type {} +// CHECK:STDOUT: class_declaration @B, () +// CHECK:STDOUT: %B: type = class_type @B +// CHECK:STDOUT: %.loc13: type = struct_type {.a: A} +// CHECK:STDOUT: %F: = fn_decl @F.2 +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: class @A { +// CHECK:STDOUT: %F: = fn_decl @F.1 +// CHECK:STDOUT: +// CHECK:STDOUT: !members: +// CHECK:STDOUT: .F = %F +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: class @B { +// CHECK:STDOUT: %A.ref: type = name_reference "A", file.%A +// CHECK:STDOUT: %.loc9: type = tuple_type () +// CHECK:STDOUT: %.loc7: type = ptr_type {} +// CHECK:STDOUT: %.loc12_8.1: type = unbound_field_type B, A +// CHECK:STDOUT: %.loc12_8.2: = field "a", member0 +// CHECK:STDOUT: %a: = bind_name "a", %.loc12_8.2 +// CHECK:STDOUT: +// CHECK:STDOUT: !members: +// CHECK:STDOUT: .a = %a +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: fn @F.1[%self.addr: A*](); +// CHECK:STDOUT: +// CHECK:STDOUT: fn @F.2(%s: {.a: A}, %b: B) { +// CHECK:STDOUT: !entry: +// CHECK:STDOUT: %.loc13: type = struct_type {.a: {}*} +// CHECK:STDOUT: %.loc11: type = ptr_type {.a: A} +// CHECK:STDOUT: %s.ref: {.a: A} = name_reference "s", %s +// CHECK:STDOUT: %.loc23_4: A = struct_access %s.ref, member0 +// CHECK:STDOUT: %.loc23_6: = bound_method %.loc23_4, @A.%F +// CHECK:STDOUT: %.loc23_8: init () = call %.loc23_6() +// CHECK:STDOUT: %b.ref: B = name_reference "b", %b +// CHECK:STDOUT: %.loc33_4.1: ref A = class_field_access %b.ref, member0 +// CHECK:STDOUT: %.loc33_4.2: A = bind_value %.loc33_4.1 +// CHECK:STDOUT: %.loc33_6: = bound_method %.loc33_4.2, @A.%F +// CHECK:STDOUT: %.loc33_8: init () = call %.loc33_6() +// CHECK:STDOUT: return +// CHECK:STDOUT: } diff --git a/toolchain/check/testdata/class/field_access_in_value.carbon b/toolchain/check/testdata/class/field_access_in_value.carbon index 833920c28337..28c83cb30442 100644 --- a/toolchain/check/testdata/class/field_access_in_value.carbon +++ b/toolchain/check/testdata/class/field_access_in_value.carbon @@ -57,9 +57,9 @@ fn Run() -> i32 { // CHECK:STDOUT: %c: Class = bind_name "c", %.loc16 // CHECK:STDOUT: %c.ref.loc17_10: Class = name_reference "c", %c // CHECK:STDOUT: %.loc17_11.1: ref i32 = class_field_access %c.ref.loc17_10, member0 +// CHECK:STDOUT: %.loc17_11.2: i32 = bind_value %.loc17_11.1 // CHECK:STDOUT: %c.ref.loc17_16: Class = name_reference "c", %c // CHECK:STDOUT: %.loc17_17.1: ref i32 = class_field_access %c.ref.loc17_16, member1 -// CHECK:STDOUT: %.loc17_11.2: i32 = bind_value %.loc17_11.1 // CHECK:STDOUT: %.loc17_17.2: i32 = bind_value %.loc17_17.1 // CHECK:STDOUT: %.loc17_14: i32 = add %.loc17_11.2, %.loc17_17.2 // CHECK:STDOUT: return %.loc17_14 diff --git a/toolchain/lower/testdata/class/value_access.carbon b/toolchain/lower/testdata/class/value_access.carbon index afc7f9452e0a..d51bec46538d 100644 --- a/toolchain/lower/testdata/class/value_access.carbon +++ b/toolchain/lower/testdata/class/value_access.carbon @@ -9,6 +9,9 @@ class C { } fn F(c: C) -> i32 { + // TODO: `c.a` is a value expression here, which forces a value binding as + // part of the member access, creating a tuple value temporary. We could + // defer performing the value binding to avoid creating this temporary. return c.a[1]; } @@ -17,7 +20,20 @@ fn F(c: C) -> i32 { // CHECK:STDOUT: // CHECK:STDOUT: define i32 @F(ptr %c) { // CHECK:STDOUT: %a = getelementptr inbounds { { i32, i32, i32 } }, ptr %c, i32 0, i32 0 -// CHECK:STDOUT: %tuple.index = getelementptr inbounds { i32, i32, i32 }, ptr %a, i32 0, i32 1 -// CHECK:STDOUT: %1 = load i32, ptr %tuple.index, align 4 -// CHECK:STDOUT: ret i32 %1 +// CHECK:STDOUT: %tuple.elem = getelementptr inbounds { i32, i32, i32 }, ptr %a, i32 0, i32 0 +// CHECK:STDOUT: %1 = load i32, ptr %tuple.elem, align 4 +// CHECK:STDOUT: %tuple.elem1 = getelementptr inbounds { i32, i32, i32 }, ptr %a, i32 0, i32 1 +// CHECK:STDOUT: %2 = load i32, ptr %tuple.elem1, align 4 +// CHECK:STDOUT: %tuple.elem2 = getelementptr inbounds { i32, i32, i32 }, ptr %a, i32 0, i32 2 +// CHECK:STDOUT: %3 = load i32, ptr %tuple.elem2, align 4 +// CHECK:STDOUT: %tuple = alloca { i32, i32, i32 }, align 8 +// CHECK:STDOUT: %4 = getelementptr inbounds { i32, i32, i32 }, ptr %tuple, i32 0, i32 0 +// CHECK:STDOUT: store i32 %1, ptr %4, align 4 +// CHECK:STDOUT: %5 = getelementptr inbounds { i32, i32, i32 }, ptr %tuple, i32 0, i32 1 +// CHECK:STDOUT: store i32 %2, ptr %5, align 4 +// CHECK:STDOUT: %6 = getelementptr inbounds { i32, i32, i32 }, ptr %tuple, i32 0, i32 2 +// CHECK:STDOUT: store i32 %3, ptr %6, align 4 +// CHECK:STDOUT: %tuple.index = getelementptr inbounds { i32, i32, i32 }, ptr %tuple, i32 0, i32 1 +// CHECK:STDOUT: %tuple.index.load = load i32, ptr %tuple.index, align 4 +// CHECK:STDOUT: ret i32 %tuple.index.load // CHECK:STDOUT: }