From 1ac7002bf6eafc5928600d273d0c1872a83239ad Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Wed, 4 Oct 2023 12:57:12 -0700 Subject: [PATCH] Don't try to infer where to put loads in lowering. (#3146) Trust semantics to have put them in the right places. Many parts of lowering still need to be updated to use the value representation chosen at the semantics layer, but this is an incremental step towards that. --- toolchain/check/handle_index.cpp | 21 +++- .../testdata/array/function_param.carbon | 6 +- .../testdata/index/expression_category.carbon | 102 ++++++++++++++++ .../index/fail_expression_category.carbon | 74 ++++++++++++ toolchain/lower/function_context.cpp | 13 +- toolchain/lower/function_context.h | 14 --- toolchain/lower/handle.cpp | 113 ++++++++++-------- .../lower/handle_expression_category.cpp | 12 ++ .../call/tuple_param_with_return_slot.carbon | 12 +- .../testdata/pointer/address_of_field.carbon | 4 +- toolchain/lower/testdata/pointer/basic.carbon | 8 +- .../pointer/pointer_to_pointer.carbon | 29 +++++ toolchain/sem_ir/file.cpp | 4 + toolchain/sem_ir/node.h | 3 + toolchain/sem_ir/node_kind.def | 2 + 15 files changed, 325 insertions(+), 92 deletions(-) create mode 100644 toolchain/check/testdata/index/expression_category.carbon create mode 100644 toolchain/check/testdata/index/fail_expression_category.carbon create mode 100644 toolchain/lower/testdata/pointer/pointer_to_pointer.carbon diff --git a/toolchain/check/handle_index.cpp b/toolchain/check/handle_index.cpp index 3de689e5b743..3426a8f45a41 100644 --- a/toolchain/check/handle_index.cpp +++ b/toolchain/check/handle_index.cpp @@ -63,9 +63,24 @@ auto HandleIndexExpression(Context& context, Parse::Node parse_node) -> bool { auto cast_index_id = ConvertToValueOfType( context, index_node.parse_node(), index_node_id, context.CanonicalizeType(SemIR::NodeId::BuiltinIntegerType)); - context.AddNodeAndPush(parse_node, SemIR::Node::ArrayIndex::Make( - parse_node, element_type_id, - operand_node_id, cast_index_id)); + auto array_cat = + SemIR::GetExpressionCategory(context.semantics_ir(), operand_node_id); + if (array_cat == SemIR::ExpressionCategory::Value) { + // If the operand is an array value, convert it to an ephemeral + // reference to an array so we can perform a primitive indexing into it. + operand_node_id = context.AddNode(SemIR::Node::ValueAsReference::Make( + parse_node, operand_type_id, operand_node_id)); + } + auto elem_id = context.AddNode(SemIR::Node::ArrayIndex::Make( + parse_node, element_type_id, operand_node_id, cast_index_id)); + if (array_cat != SemIR::ExpressionCategory::DurableReference) { + // Indexing a durable reference gives a durable reference expression. + // Indexing anything else gives a value expression. + // TODO: This should be replaced by a choice between using `IndexWith` + // and `IndirectIndexWith`. + elem_id = ConvertToValueExpression(context, elem_id); + } + context.node_stack().Push(parse_node, elem_id); return true; } case SemIR::NodeKind::TupleType: { diff --git a/toolchain/check/testdata/array/function_param.carbon b/toolchain/check/testdata/array/function_param.carbon index 7b45de272b63..5b6a848ceb78 100644 --- a/toolchain/check/testdata/array/function_param.carbon +++ b/toolchain/check/testdata/array/function_param.carbon @@ -19,8 +19,10 @@ fn G() -> i32 { // CHECK:STDOUT: // CHECK:STDOUT: fn @F(%arr: [i32; 3], %i: i32) -> i32 { // CHECK:STDOUT: !entry: -// CHECK:STDOUT: %.loc8: i32 = array_index %arr, %i -// CHECK:STDOUT: return %.loc8 +// CHECK:STDOUT: %.loc8_15.1: ref [i32; 3] = value_as_reference %arr +// CHECK:STDOUT: %.loc8_15.2: ref i32 = array_index %.loc8_15.1, %i +// CHECK:STDOUT: %.loc8_15.3: i32 = bind_value %.loc8_15.2 +// CHECK:STDOUT: return %.loc8_15.3 // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: fn @G() -> i32 { diff --git a/toolchain/check/testdata/index/expression_category.carbon b/toolchain/check/testdata/index/expression_category.carbon new file mode 100644 index 000000000000..20509672eda6 --- /dev/null +++ b/toolchain/check/testdata/index/expression_category.carbon @@ -0,0 +1,102 @@ +// 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 + +fn F() -> [i32; 3]; + +fn G(b: [i32; 3]) { + var a: [i32; 3] = (1, 2, 3); + + // Indexing a durable array reference gives a durable reference. + var pa: i32* = &a[0]; + a[0] = 4; +} + +fn ValueBinding(b: [i32; 3]) { + var a: [i32; 3] = (1, 2, 3); + + // Index but don't do anything else so we can check that a value binding is + // produced when appropriate. + a[0]; + b[0]; + F()[0]; +} + +// CHECK:STDOUT: file "expression_category.carbon" { +// CHECK:STDOUT: %.loc7 = fn_decl @F +// CHECK:STDOUT: %.loc9 = fn_decl @G +// CHECK:STDOUT: %.loc17 = fn_decl @ValueBinding +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: fn @F() -> %return: [i32; 3]; +// CHECK:STDOUT: +// CHECK:STDOUT: fn @G(%b: [i32; 3]) { +// CHECK:STDOUT: !entry: +// CHECK:STDOUT: %.loc10_16: i32 = int_literal 3 +// CHECK:STDOUT: %.loc10_17: type = array_type %.loc10_16, i32 +// CHECK:STDOUT: %a: ref [i32; 3] = var "a" +// CHECK:STDOUT: %.loc10_22: i32 = int_literal 1 +// CHECK:STDOUT: %.loc10_25: i32 = int_literal 2 +// CHECK:STDOUT: %.loc10_28: i32 = int_literal 3 +// CHECK:STDOUT: %.loc10_29.1: type = tuple_type (i32, i32, i32) +// CHECK:STDOUT: %.loc10_29.2: (i32, i32, i32) = tuple_literal (%.loc10_22, %.loc10_25, %.loc10_28) +// CHECK:STDOUT: %.loc10_29.3: i32 = int_literal 0 +// CHECK:STDOUT: %.loc10_29.4: ref i32 = array_index %a, %.loc10_29.3 +// CHECK:STDOUT: %.loc10_29.5: init i32 = initialize_from %.loc10_22 to %.loc10_29.4 +// CHECK:STDOUT: %.loc10_29.6: i32 = int_literal 1 +// CHECK:STDOUT: %.loc10_29.7: ref i32 = array_index %a, %.loc10_29.6 +// CHECK:STDOUT: %.loc10_29.8: init i32 = initialize_from %.loc10_25 to %.loc10_29.7 +// CHECK:STDOUT: %.loc10_29.9: i32 = int_literal 2 +// CHECK:STDOUT: %.loc10_29.10: ref i32 = array_index %a, %.loc10_29.9 +// CHECK:STDOUT: %.loc10_29.11: init i32 = initialize_from %.loc10_28 to %.loc10_29.10 +// CHECK:STDOUT: %.loc10_29.12: init [i32; 3] = array_init %.loc10_29.2, (%.loc10_29.5, %.loc10_29.8, %.loc10_29.11) to %a +// CHECK:STDOUT: assign %a, %.loc10_29.12 +// CHECK:STDOUT: %.loc13_14: type = ptr_type i32 +// CHECK:STDOUT: %pa: ref i32* = var "pa" +// CHECK:STDOUT: %.loc13_21: i32 = int_literal 0 +// CHECK:STDOUT: %.loc13_22: ref i32 = array_index %a, %.loc13_21 +// CHECK:STDOUT: %.loc13_18: i32* = address_of %.loc13_22 +// CHECK:STDOUT: assign %pa, %.loc13_18 +// CHECK:STDOUT: %.loc14_5: i32 = int_literal 0 +// CHECK:STDOUT: %.loc14_6: ref i32 = array_index %a, %.loc14_5 +// CHECK:STDOUT: %.loc14_10: i32 = int_literal 4 +// CHECK:STDOUT: assign %.loc14_6, %.loc14_10 +// CHECK:STDOUT: return +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: fn @ValueBinding(%b: [i32; 3]) { +// CHECK:STDOUT: !entry: +// CHECK:STDOUT: %.loc18_16: i32 = int_literal 3 +// CHECK:STDOUT: %.loc18_17: type = array_type %.loc18_16, i32 +// CHECK:STDOUT: %a: ref [i32; 3] = var "a" +// CHECK:STDOUT: %.loc18_22: i32 = int_literal 1 +// CHECK:STDOUT: %.loc18_25: i32 = int_literal 2 +// CHECK:STDOUT: %.loc18_28: i32 = int_literal 3 +// CHECK:STDOUT: %.loc18_29.1: (i32, i32, i32) = tuple_literal (%.loc18_22, %.loc18_25, %.loc18_28) +// CHECK:STDOUT: %.loc18_29.2: i32 = int_literal 0 +// CHECK:STDOUT: %.loc18_29.3: ref i32 = array_index %a, %.loc18_29.2 +// CHECK:STDOUT: %.loc18_29.4: init i32 = initialize_from %.loc18_22 to %.loc18_29.3 +// CHECK:STDOUT: %.loc18_29.5: i32 = int_literal 1 +// CHECK:STDOUT: %.loc18_29.6: ref i32 = array_index %a, %.loc18_29.5 +// CHECK:STDOUT: %.loc18_29.7: init i32 = initialize_from %.loc18_25 to %.loc18_29.6 +// CHECK:STDOUT: %.loc18_29.8: i32 = int_literal 2 +// CHECK:STDOUT: %.loc18_29.9: ref i32 = array_index %a, %.loc18_29.8 +// CHECK:STDOUT: %.loc18_29.10: init i32 = initialize_from %.loc18_28 to %.loc18_29.9 +// CHECK:STDOUT: %.loc18_29.11: init [i32; 3] = array_init %.loc18_29.1, (%.loc18_29.4, %.loc18_29.7, %.loc18_29.10) to %a +// CHECK:STDOUT: assign %a, %.loc18_29.11 +// CHECK:STDOUT: %.loc22_5: i32 = int_literal 0 +// CHECK:STDOUT: %.loc22_6: ref i32 = array_index %a, %.loc22_5 +// CHECK:STDOUT: %.loc23_5: i32 = int_literal 0 +// CHECK:STDOUT: %.loc23_6.1: ref [i32; 3] = value_as_reference %b +// CHECK:STDOUT: %.loc23_6.2: ref i32 = array_index %.loc23_6.1, %.loc23_5 +// CHECK:STDOUT: %.loc23_6.3: i32 = bind_value %.loc23_6.2 +// CHECK:STDOUT: %.loc24_4.1: ref [i32; 3] = temporary_storage +// CHECK:STDOUT: %.loc24_4.2: init [i32; 3] = call @F() to %.loc24_4.1 +// CHECK:STDOUT: %.loc24_7: i32 = int_literal 0 +// CHECK:STDOUT: %.loc24_4.3: ref [i32; 3] = temporary %.loc24_4.1, %.loc24_4.2 +// CHECK:STDOUT: %.loc24_8.1: ref i32 = array_index %.loc24_4.3, %.loc24_7 +// CHECK:STDOUT: %.loc24_8.2: i32 = bind_value %.loc24_8.1 +// CHECK:STDOUT: return +// CHECK:STDOUT: } diff --git a/toolchain/check/testdata/index/fail_expression_category.carbon b/toolchain/check/testdata/index/fail_expression_category.carbon new file mode 100644 index 000000000000..199cd3ae3e28 --- /dev/null +++ b/toolchain/check/testdata/index/fail_expression_category.carbon @@ -0,0 +1,74 @@ +// 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 + +fn F() -> [i32; 3]; + +fn G(b: [i32; 3]) { + // Indexing an array value gives a value. + // CHECK:STDERR: fail_expression_category.carbon:[[@LINE+3]]:18: ERROR: Cannot take the address of non-reference expression. + // CHECK:STDERR: var pb: i32* = &b[0]; + // CHECK:STDERR: ^ + var pb: i32* = &b[0]; + // CHECK:STDERR: fail_expression_category.carbon:[[@LINE+3]]:6: ERROR: Expression is not assignable. + // CHECK:STDERR: b[0] = 4; + // CHECK:STDERR: ^ + b[0] = 4; + + // Indexing an ephemeral reference (materialized from an initializing + // expression) gives a value. + // CHECK:STDERR: fail_expression_category.carbon:[[@LINE+3]]:18: ERROR: Cannot take the address of non-reference expression. + // CHECK:STDERR: var pf: i32* = &F()[0]; + // CHECK:STDERR: ^ + var pf: i32* = &F()[0]; + // CHECK:STDERR: fail_expression_category.carbon:[[@LINE+3]]:8: ERROR: Expression is not assignable. + // CHECK:STDERR: F()[0] = 4; + // CHECK:STDERR: ^ + F()[0] = 4; +} + +// CHECK:STDOUT: file "fail_expression_category.carbon" { +// CHECK:STDOUT: %.loc7 = fn_decl @F +// CHECK:STDOUT: %.loc9 = fn_decl @G +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: fn @F() -> %return: [i32; 3]; +// CHECK:STDOUT: +// CHECK:STDOUT: fn @G(%b: [i32; 3]) { +// CHECK:STDOUT: !entry: +// CHECK:STDOUT: %.loc14_14: type = ptr_type i32 +// CHECK:STDOUT: %pb: ref i32* = var "pb" +// CHECK:STDOUT: %.loc14_21: i32 = int_literal 0 +// CHECK:STDOUT: %.loc14_22.1: ref [i32; 3] = value_as_reference %b +// CHECK:STDOUT: %.loc14_22.2: ref i32 = array_index %.loc14_22.1, %.loc14_21 +// CHECK:STDOUT: %.loc14_22.3: i32 = bind_value %.loc14_22.2 +// CHECK:STDOUT: %.loc14_18: i32* = address_of %.loc14_22.3 +// CHECK:STDOUT: assign %pb, %.loc14_18 +// CHECK:STDOUT: %.loc18_5: i32 = int_literal 0 +// CHECK:STDOUT: %.loc18_6.1: ref [i32; 3] = value_as_reference %b +// CHECK:STDOUT: %.loc18_6.2: ref i32 = array_index %.loc18_6.1, %.loc18_5 +// CHECK:STDOUT: %.loc18_6.3: i32 = bind_value %.loc18_6.2 +// CHECK:STDOUT: %.loc18_10: i32 = int_literal 4 +// CHECK:STDOUT: assign %.loc18_6.3, %.loc18_10 +// CHECK:STDOUT: %.loc25_14: type = ptr_type i32 +// CHECK:STDOUT: %pf: ref i32* = var "pf" +// CHECK:STDOUT: %.loc25_20.1: ref [i32; 3] = temporary_storage +// CHECK:STDOUT: %.loc25_20.2: init [i32; 3] = call @F() to %.loc25_20.1 +// CHECK:STDOUT: %.loc25_23: i32 = int_literal 0 +// CHECK:STDOUT: %.loc25_20.3: ref [i32; 3] = temporary %.loc25_20.1, %.loc25_20.2 +// CHECK:STDOUT: %.loc25_24.1: ref i32 = array_index %.loc25_20.3, %.loc25_23 +// CHECK:STDOUT: %.loc25_24.2: i32 = bind_value %.loc25_24.1 +// CHECK:STDOUT: %.loc25_18: i32* = address_of %.loc25_24.2 +// CHECK:STDOUT: assign %pf, %.loc25_18 +// CHECK:STDOUT: %.loc29_4.1: ref [i32; 3] = temporary_storage +// CHECK:STDOUT: %.loc29_4.2: init [i32; 3] = call @F() to %.loc29_4.1 +// CHECK:STDOUT: %.loc29_7: i32 = int_literal 0 +// CHECK:STDOUT: %.loc29_4.3: ref [i32; 3] = temporary %.loc29_4.1, %.loc29_4.2 +// CHECK:STDOUT: %.loc29_8.1: ref i32 = array_index %.loc29_4.3, %.loc29_7 +// CHECK:STDOUT: %.loc29_8.2: i32 = bind_value %.loc29_8.1 +// CHECK:STDOUT: %.loc29_12: i32 = int_literal 4 +// CHECK:STDOUT: assign %.loc29_8.2, %.loc29_12 +// CHECK:STDOUT: return +// CHECK:STDOUT: } diff --git a/toolchain/lower/function_context.cpp b/toolchain/lower/function_context.cpp index 839361c90acc..320136235010 100644 --- a/toolchain/lower/function_context.cpp +++ b/toolchain/lower/function_context.cpp @@ -98,7 +98,7 @@ auto FunctionContext::CopyValue(SemIR::TypeId type_id, SemIR::NodeId source_id, case SemIR::ValueRepresentation::None: break; case SemIR::ValueRepresentation::Copy: - builder().CreateStore(GetLocalLoaded(source_id), GetLocal(dest_id)); + builder().CreateStore(GetLocal(source_id), GetLocal(dest_id)); break; case SemIR::ValueRepresentation::Pointer: { const auto& layout = llvm_module().getDataLayout(); @@ -118,15 +118,4 @@ auto FunctionContext::CopyValue(SemIR::TypeId type_id, SemIR::NodeId source_id, } } -auto FunctionContext::GetLocalLoaded(SemIR::NodeId node_id) -> llvm::Value* { - auto* value = GetLocal(node_id); - if (llvm::isa(value)) { - auto* load_type = GetType(semantics_ir().GetNode(node_id).type_id()); - return builder().CreateLoad(load_type, value); - } else { - // No load is needed. - return value; - } -} - } // namespace Carbon::Lower diff --git a/toolchain/lower/function_context.h b/toolchain/lower/function_context.h index 9b2df0b2f48f..5bba15aec949 100644 --- a/toolchain/lower/function_context.h +++ b/toolchain/lower/function_context.h @@ -51,26 +51,12 @@ class FunctionContext { return it->second; } - // Returns a local (versus global) value for the given node in loaded state. - // Loads will only be inserted on an as-needed basis. - auto GetLocalLoaded(SemIR::NodeId node_id) -> llvm::Value*; - // Sets the value for the given node. auto SetLocal(SemIR::NodeId node_id, llvm::Value* value) { bool added = locals_.insert({node_id, value}).second; CARBON_CHECK(added) << "Duplicate local insert: " << node_id; } - // Returns the requested index into val based on whether val is a pointer - // type. - auto GetIndexFromStructOrArray(llvm::Type* llvm_type, llvm::Value* val, - unsigned idx, const llvm::Twine& name) - -> llvm::Value* { - return val->getType()->isPointerTy() - ? builder().CreateStructGEP(llvm_type, val, idx, name) - : builder().CreateExtractValue(val, idx, name); - } - // Gets a callable's function. auto GetFunction(SemIR::FunctionId function_id) -> llvm::Function* { return file_context_->GetFunction(function_id); diff --git a/toolchain/lower/handle.cpp b/toolchain/lower/handle.cpp index c93b0458575f..9ef4dc2f6bcc 100644 --- a/toolchain/lower/handle.cpp +++ b/toolchain/lower/handle.cpp @@ -30,24 +30,12 @@ auto HandleArrayIndex(FunctionContext& context, SemIR::NodeId node_id, auto* array_value = context.GetLocal(array_node_id); auto* llvm_type = context.GetType(context.semantics_ir().GetNode(array_node_id).type_id()); - auto index_node = context.semantics_ir().GetNode(index_node_id); - llvm::Value* array_element_value; - - if (index_node.kind() == SemIR::NodeKind::IntegerLiteral) { - const auto index = context.semantics_ir() - .GetIntegerLiteral(index_node.GetAsIntegerLiteral()) - .getZExtValue(); - array_element_value = context.GetIndexFromStructOrArray( - llvm_type, array_value, index, "array.index"); - } else { - auto* index = context.GetLocalLoaded(index_node_id); - // TODO: Handle return value or call such as `F()[a]`. - auto* zero = llvm::ConstantInt::get( - llvm::Type::getInt32Ty(context.llvm_context()), 0); - array_element_value = context.builder().CreateInBoundsGEP( - llvm_type, array_value, {zero, index}, "array.index"); - } - context.SetLocal(node_id, array_element_value); + llvm::Value* indexes[2] = { + llvm::ConstantInt::get(llvm::Type::getInt32Ty(context.llvm_context()), 0), + context.GetLocal(index_node_id)}; + context.SetLocal(node_id, + context.builder().CreateInBoundsGEP(llvm_type, array_value, + indexes, "array.index")); } auto HandleArrayInit(FunctionContext& context, SemIR::NodeId node_id, @@ -108,7 +96,7 @@ auto HandleBranch(FunctionContext& context, SemIR::NodeId /*node_id*/, auto HandleBranchIf(FunctionContext& context, SemIR::NodeId /*node_id*/, SemIR::Node node) -> void { auto [target_block_id, cond_id] = node.GetAsBranchIf(); - llvm::Value* cond = context.GetLocalLoaded(cond_id); + llvm::Value* cond = context.GetLocal(cond_id); llvm::BasicBlock* then_block = context.GetBlock(target_block_id); llvm::BasicBlock* else_block = context.CreateSyntheticBlock(); context.builder().CreateCondBr(cond, then_block, else_block); @@ -118,7 +106,7 @@ auto HandleBranchIf(FunctionContext& context, SemIR::NodeId /*node_id*/, auto HandleBranchWithArg(FunctionContext& context, SemIR::NodeId /*node_id*/, SemIR::Node node) -> void { auto [target_block_id, arg_id] = node.GetAsBranchWithArg(); - llvm::Value* arg = context.GetLocalLoaded(arg_id); + llvm::Value* arg = context.GetLocal(arg_id); SemIR::TypeId arg_type_id = context.semantics_ir().GetNode(arg_id).type_id(); // Opportunistically avoid creating a BasicBlock that contains just a branch. @@ -164,17 +152,9 @@ auto HandleCall(FunctionContext& context, SemIR::NodeId node_id, for (auto ref_id : arg_ids) { auto arg_type_id = context.semantics_ir().GetNode(ref_id).type_id(); - switch (SemIR::GetValueRepresentation(context.semantics_ir(), arg_type_id) - .kind) { - case SemIR::ValueRepresentation::None: - break; - case SemIR::ValueRepresentation::Copy: - case SemIR::ValueRepresentation::Custom: - args.push_back(context.GetLocalLoaded(ref_id)); - break; - case SemIR::ValueRepresentation::Pointer: - args.push_back(context.GetLocal(ref_id)); - break; + if (SemIR::GetValueRepresentation(context.semantics_ir(), arg_type_id) + .kind != SemIR::ValueRepresentation::None) { + args.push_back(context.GetLocal(ref_id)); } } @@ -270,7 +250,7 @@ auto HandleReturnExpression(FunctionContext& context, SemIR::NodeId /*node_id*/, return; case SemIR::InitializingRepresentation::ByCopy: // The expression produces the value representation for the type. - context.builder().CreateRet(context.GetLocalLoaded(expr_id)); + context.builder().CreateRet(context.GetLocal(expr_id)); return; } } @@ -287,11 +267,55 @@ auto HandleStringLiteral(FunctionContext& /*context*/, CARBON_FATAL() << "TODO: Add support: " << node; } +// Extracts an element of either a struct or a tuple by index. Depending on the +// expression category of the aggregate input, this will either produce a value +// or a reference. +static auto GetStructOrTupleElement(FunctionContext& context, + SemIR::NodeId aggr_node_id, unsigned idx, + SemIR::TypeId result_type_id, + llvm::Twine name) -> llvm::Value* { + auto aggr_node = context.semantics_ir().GetNode(aggr_node_id); + auto* aggr_value = context.GetLocal(aggr_node_id); + + auto aggr_cat = + SemIR::GetExpressionCategory(context.semantics_ir(), aggr_node_id); + if (aggr_cat == SemIR::ExpressionCategory::Value && + SemIR::GetValueRepresentation(context.semantics_ir(), aggr_node.type_id()) + .kind == SemIR::ValueRepresentation::Copy) { + // We are holding the values of the aggregate directly, elementwise. + return context.builder().CreateExtractValue(aggr_value, idx, name); + } + + // Either we're accessing an element of a reference and producing a reference, + // or we're accessing an element of a value that is held by pointer and we're + // producing a value. + auto* aggr_type = context.GetType(aggr_node.type_id()); + auto* elem_ptr = + context.builder().CreateStructGEP(aggr_type, aggr_value, idx, name); + + // If this is a value access, load the element if necessary. + if (aggr_cat == SemIR::ExpressionCategory::Value) { + switch ( + SemIR::GetValueRepresentation(context.semantics_ir(), result_type_id) + .kind) { + case SemIR::ValueRepresentation::None: + return llvm::PoisonValue::get(context.GetType(result_type_id)); + case SemIR::ValueRepresentation::Copy: + return context.builder().CreateLoad(context.GetType(result_type_id), + elem_ptr, name + ".load"); + case SemIR::ValueRepresentation::Pointer: + return elem_ptr; + case SemIR::ValueRepresentation::Custom: + CARBON_FATAL() << "TODO: Add support for custom value representation"; + } + } + return elem_ptr; +} + auto HandleStructAccess(FunctionContext& context, SemIR::NodeId node_id, SemIR::Node node) -> void { auto [struct_id, member_index] = node.GetAsStructAccess(); auto struct_type_id = context.semantics_ir().GetNode(struct_id).type_id(); - auto* llvm_type = context.GetType(struct_type_id); // Get type information for member names. auto type_refs = context.semantics_ir().GetNodeBlock( @@ -304,9 +328,9 @@ auto HandleStructAccess(FunctionContext& context, SemIR::NodeId node_id, .GetAsStructTypeField(); auto member_name = context.semantics_ir().GetString(field_name_id); - auto* gep = context.builder().CreateStructGEP( - llvm_type, context.GetLocal(struct_id), member_index.index, member_name); - context.SetLocal(node_id, gep); + context.SetLocal( + node_id, GetStructOrTupleElement(context, struct_id, member_index.index, + node.type_id(), member_name)); } auto HandleStructLiteral(FunctionContext& context, SemIR::NodeId node_id, @@ -400,26 +424,21 @@ auto HandleStructTypeField(FunctionContext& /*context*/, auto HandleTupleAccess(FunctionContext& context, SemIR::NodeId node_id, SemIR::Node node) -> void { auto [tuple_node_id, index] = node.GetAsTupleAccess(); - auto* tuple_value = context.GetLocal(tuple_node_id); - auto* llvm_type = - context.GetType(context.semantics_ir().GetNode(tuple_node_id).type_id()); - context.SetLocal( - node_id, context.GetIndexFromStructOrArray(llvm_type, tuple_value, - index.index, "tuple.elem")); + context.SetLocal(node_id, + GetStructOrTupleElement(context, tuple_node_id, index.index, + node.type_id(), "tuple.elem")); } auto HandleTupleIndex(FunctionContext& context, SemIR::NodeId node_id, SemIR::Node node) -> void { auto [tuple_node_id, index_node_id] = node.GetAsTupleIndex(); - auto* tuple_value = context.GetLocal(tuple_node_id); auto index_node = context.semantics_ir().GetNode(index_node_id); const auto index = context.semantics_ir() .GetIntegerLiteral(index_node.GetAsIntegerLiteral()) .getZExtValue(); - auto* llvm_type = - context.GetType(context.semantics_ir().GetNode(tuple_node_id).type_id()); - context.SetLocal(node_id, context.GetIndexFromStructOrArray( - llvm_type, tuple_value, index, "tuple.index")); + context.SetLocal(node_id, + GetStructOrTupleElement(context, tuple_node_id, index, + node.type_id(), "tuple.index")); } auto HandleTupleLiteral(FunctionContext& context, SemIR::NodeId node_id, diff --git a/toolchain/lower/handle_expression_category.cpp b/toolchain/lower/handle_expression_category.cpp index 3b48becb8e59..8c8faa1c91e6 100644 --- a/toolchain/lower/handle_expression_category.cpp +++ b/toolchain/lower/handle_expression_category.cpp @@ -3,6 +3,7 @@ // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception #include "toolchain/lower/function_context.h" +#include "toolchain/sem_ir/file.h" namespace Carbon::Lower { @@ -45,4 +46,15 @@ auto HandleTemporaryStorage(FunctionContext& context, SemIR::NodeId node_id, nullptr, "temp")); } +auto HandleValueAsReference(FunctionContext& context, SemIR::NodeId node_id, + SemIR::Node node) -> void { + CARBON_CHECK(SemIR::GetExpressionCategory(context.semantics_ir(), + node.GetAsValueAsReference()) == + SemIR::ExpressionCategory::Value); + CARBON_CHECK( + SemIR::GetValueRepresentation(context.semantics_ir(), node.type_id()) + .kind == SemIR::ValueRepresentation::Pointer); + context.SetLocal(node_id, context.GetLocal(node.GetAsValueAsReference())); +} + } // namespace Carbon::Lower diff --git a/toolchain/lower/testdata/function/call/tuple_param_with_return_slot.carbon b/toolchain/lower/testdata/function/call/tuple_param_with_return_slot.carbon index bf90f7c10d9e..d8b8c38cb485 100644 --- a/toolchain/lower/testdata/function/call/tuple_param_with_return_slot.carbon +++ b/toolchain/lower/testdata/function/call/tuple_param_with_return_slot.carbon @@ -18,15 +18,15 @@ fn Main() { // CHECK:STDOUT: define void @F(ptr sret({ i32, i32, i32 }) %return, { i32 } %b, ptr %c) { // CHECK:STDOUT: %tuple.index = extractvalue { i32 } %b, 0 // CHECK:STDOUT: %tuple.index1 = getelementptr inbounds { i32, i32 }, ptr %c, i32 0, i32 0 +// CHECK:STDOUT: %tuple.index.load = load i32, ptr %tuple.index1, align 4 // CHECK:STDOUT: %tuple.index2 = getelementptr inbounds { i32, i32 }, ptr %c, i32 0, i32 1 +// CHECK:STDOUT: %tuple.index.load3 = load i32, ptr %tuple.index2, align 4 // CHECK:STDOUT: %tuple.elem = getelementptr inbounds { i32, i32, i32 }, ptr %return, i32 0, i32 0 // CHECK:STDOUT: store i32 %tuple.index, ptr %tuple.elem, align 4 -// CHECK:STDOUT: %tuple.elem3 = getelementptr inbounds { i32, i32, i32 }, ptr %return, i32 0, i32 1 -// CHECK:STDOUT: %1 = load i32, ptr %tuple.index1, align 4 -// CHECK:STDOUT: store i32 %1, ptr %tuple.elem3, align 4 -// CHECK:STDOUT: %tuple.elem4 = getelementptr inbounds { i32, i32, i32 }, ptr %return, i32 0, i32 2 -// CHECK:STDOUT: %2 = load i32, ptr %tuple.index2, align 4 -// CHECK:STDOUT: store i32 %2, ptr %tuple.elem4, align 4 +// CHECK:STDOUT: %tuple.elem4 = getelementptr inbounds { i32, i32, i32 }, ptr %return, i32 0, i32 1 +// CHECK:STDOUT: store i32 %tuple.index.load, ptr %tuple.elem4, align 4 +// CHECK:STDOUT: %tuple.elem5 = getelementptr inbounds { i32, i32, i32 }, ptr %return, i32 0, i32 2 +// CHECK:STDOUT: store i32 %tuple.index.load3, ptr %tuple.elem5, align 4 // CHECK:STDOUT: ret void // CHECK:STDOUT: } // CHECK:STDOUT: diff --git a/toolchain/lower/testdata/pointer/address_of_field.carbon b/toolchain/lower/testdata/pointer/address_of_field.carbon index bce0c86a58ff..b08d6145532e 100644 --- a/toolchain/lower/testdata/pointer/address_of_field.carbon +++ b/toolchain/lower/testdata/pointer/address_of_field.carbon @@ -8,7 +8,6 @@ fn G(p: i32*); fn F() { var s: {.a: i32, .b: i32} = {.a = 1, .b = 2}; - // TODO: The lowering here is incorrect: we're incorrectly loading `s.b`. G(&s.b); } @@ -24,7 +23,6 @@ fn F() { // CHECK:STDOUT: %b = getelementptr inbounds { i32, i32 }, ptr %s, i32 0, i32 1 // CHECK:STDOUT: store i32 2, ptr %b, align 4 // CHECK:STDOUT: %b1 = getelementptr inbounds { i32, i32 }, ptr %s, i32 0, i32 1 -// CHECK:STDOUT: %1 = load ptr, ptr %b1, align 8 -// CHECK:STDOUT: call void @G(ptr %1) +// CHECK:STDOUT: call void @G(ptr %b1) // CHECK:STDOUT: ret void // CHECK:STDOUT: } diff --git a/toolchain/lower/testdata/pointer/basic.carbon b/toolchain/lower/testdata/pointer/basic.carbon index ae24f7c6c872..82687f1105af 100644 --- a/toolchain/lower/testdata/pointer/basic.carbon +++ b/toolchain/lower/testdata/pointer/basic.carbon @@ -5,7 +5,6 @@ // AUTOUPDATE fn G(p: i32*) -> i32 { - // TODO: The LLVM IR generated here is wrong, missing a load. return *p; } @@ -25,10 +24,9 @@ fn F() -> i32 { // CHECK:STDOUT: define i32 @F() { // CHECK:STDOUT: %n = alloca i32, align 4 // CHECK:STDOUT: store i32 0, ptr %n, align 4 -// CHECK:STDOUT: %1 = load ptr, ptr %n, align 8 -// CHECK:STDOUT: %G = call i32 @G(ptr %1) +// CHECK:STDOUT: %G = call i32 @G(ptr %n) // CHECK:STDOUT: %temp = alloca i32, align 4 // CHECK:STDOUT: store i32 %G, ptr %temp, align 4 -// CHECK:STDOUT: %2 = load i32, ptr %temp, align 4 -// CHECK:STDOUT: ret i32 %2 +// CHECK:STDOUT: %1 = load i32, ptr %temp, align 4 +// CHECK:STDOUT: ret i32 %1 // CHECK:STDOUT: } diff --git a/toolchain/lower/testdata/pointer/pointer_to_pointer.carbon b/toolchain/lower/testdata/pointer/pointer_to_pointer.carbon new file mode 100644 index 000000000000..aa342072eaf5 --- /dev/null +++ b/toolchain/lower/testdata/pointer/pointer_to_pointer.carbon @@ -0,0 +1,29 @@ +// 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 + +fn F(p: i32**) -> i32 { + var a: i32** = p; + var b: i32* = *p; + var c: i32** = &b; + return **c; +} + +// CHECK:STDOUT: ; ModuleID = 'pointer_to_pointer.carbon' +// CHECK:STDOUT: source_filename = "pointer_to_pointer.carbon" +// CHECK:STDOUT: +// CHECK:STDOUT: define i32 @F(ptr %p) { +// CHECK:STDOUT: %a = alloca ptr, align 8 +// CHECK:STDOUT: store ptr %p, ptr %a, align 8 +// CHECK:STDOUT: %b = alloca ptr, align 8 +// CHECK:STDOUT: %1 = load ptr, ptr %p, align 8 +// CHECK:STDOUT: store ptr %1, ptr %b, align 8 +// CHECK:STDOUT: %c = alloca ptr, align 8 +// CHECK:STDOUT: store ptr %b, ptr %c, align 8 +// CHECK:STDOUT: %2 = load ptr, ptr %c, align 8 +// CHECK:STDOUT: %3 = load ptr, ptr %2, align 8 +// CHECK:STDOUT: %4 = load i32, ptr %3, align 4 +// CHECK:STDOUT: ret i32 %4 +// CHECK:STDOUT: } diff --git a/toolchain/sem_ir/file.cpp b/toolchain/sem_ir/file.cpp index f230c2ab45e2..c0d79b921a99 100644 --- a/toolchain/sem_ir/file.cpp +++ b/toolchain/sem_ir/file.cpp @@ -231,6 +231,7 @@ static auto GetTypePrecedence(NodeKind kind) -> int { case NodeKind::TupleInit: case NodeKind::TupleValue: case NodeKind::UnaryOperatorNot: + case NodeKind::ValueAsReference: case NodeKind::VarStorage: CARBON_FATAL() << "GetTypePrecedence for non-type node kind " << kind; } @@ -398,6 +399,7 @@ auto File::StringifyType(TypeId type_id, bool in_type_context) const case NodeKind::TupleInit: case NodeKind::TupleValue: case NodeKind::UnaryOperatorNot: + case NodeKind::ValueAsReference: case NodeKind::VarStorage: // We don't need to handle stringification for nodes that don't show up // in errors, but make it clear what's going on so that it's clearer @@ -518,6 +520,7 @@ auto GetExpressionCategory(const File& file, NodeId node_id) case NodeKind::Temporary: case NodeKind::TemporaryStorage: + case NodeKind::ValueAsReference: return ExpressionCategory::EphemeralReference; } } @@ -569,6 +572,7 @@ auto GetValueRepresentation(const File& file, TypeId type_id) case NodeKind::TupleInit: case NodeKind::TupleValue: case NodeKind::UnaryOperatorNot: + case NodeKind::ValueAsReference: case NodeKind::VarStorage: CARBON_FATAL() << "Type refers to non-type node " << node; diff --git a/toolchain/sem_ir/node.h b/toolchain/sem_ir/node.h index 48fd2984aa7f..749009326333 100644 --- a/toolchain/sem_ir/node.h +++ b/toolchain/sem_ir/node.h @@ -449,6 +449,9 @@ class Node : public Printable { using UnaryOperatorNot = Factory; + using ValueAsReference = + Factory; + using VarStorage = Factory; explicit Node() diff --git a/toolchain/sem_ir/node_kind.def b/toolchain/sem_ir/node_kind.def index 4dda72a6c06d..779036893009 100644 --- a/toolchain/sem_ir/node_kind.def +++ b/toolchain/sem_ir/node_kind.def @@ -103,6 +103,8 @@ CARBON_SEMANTICS_NODE_KIND_IMPL(TupleLiteral, "tuple_literal", Typed, CARBON_SEMANTICS_NODE_KIND_IMPL(TupleType, "tuple_type", Typed, NotTerminator) CARBON_SEMANTICS_NODE_KIND_IMPL(TupleValue, "tuple_value", Typed, NotTerminator) CARBON_SEMANTICS_NODE_KIND_IMPL(UnaryOperatorNot, "not", Typed, NotTerminator) +CARBON_SEMANTICS_NODE_KIND_IMPL(ValueAsReference, "value_as_reference", Typed, + NotTerminator) CARBON_SEMANTICS_NODE_KIND_IMPL(VarStorage, "var", Typed, NotTerminator) #undef CARBON_SEMANTICS_NODE_KIND