From 46341f015d1a478a9a59fb871dc521be2a914325 Mon Sep 17 00:00:00 2001 From: Michael Haselberger Date: Tue, 28 Jan 2025 16:48:04 +0100 Subject: [PATCH] review feedback(aggregate return lowering) --- compiler/plc_ast/src/ast.rs | 12 +- src/codegen/debug.rs | 2 +- src/codegen/generators/pou_generator.rs | 6 +- src/codegen/generators/statement_generator.rs | 10 +- src/codegen/tests/code_gen_tests.rs | 2 - src/index.rs | 4 +- src/lowering/calls.rs | 126 +++++++++++------- src/resolver.rs | 27 +--- src/validation/tests/pou_validation_tests.rs | 13 +- 9 files changed, 99 insertions(+), 103 deletions(-) diff --git a/compiler/plc_ast/src/ast.rs b/compiler/plc_ast/src/ast.rs index c5253394f1..66ec84ab9f 100644 --- a/compiler/plc_ast/src/ast.rs +++ b/compiler/plc_ast/src/ast.rs @@ -236,7 +236,7 @@ impl Pou { } pub fn calc_return_name(pou_name: &str) -> &str { - pou_name.split('.').last().unwrap_or_default() + pou_name.rsplit_once('.').map(|(_, return_name)| return_name).unwrap_or(pou_name) } pub fn is_aggregate(&self) -> bool { @@ -317,7 +317,7 @@ impl PouType { } } - pub fn is_function_or_init(&self) -> bool { + pub fn is_function_method_or_init(&self) -> bool { matches!(self, PouType::Function | PouType::Init | PouType::ProjectInit | PouType::Method { .. }) } } @@ -538,7 +538,7 @@ impl From<&DataTypeDeclaration> for SourceLocation { impl DataTypeDeclaration { pub fn get_name(&self) -> Option<&str> { match self { - Self::Aggregate { referenced_type, .. } + DataTypeDeclaration::Aggregate { referenced_type, .. } | DataTypeDeclaration::DataTypeReference { referenced_type, .. } => { Some(referenced_type.as_str()) } @@ -550,7 +550,7 @@ impl DataTypeDeclaration { match self { DataTypeDeclaration::DataTypeReference { location, .. } => location.clone(), DataTypeDeclaration::DataTypeDefinition { location, .. } => location.clone(), - Self::Aggregate { location, .. } => location.clone(), + DataTypeDeclaration::Aggregate { location, .. } => location.clone(), } } @@ -570,12 +570,12 @@ impl DataTypeDeclaration { None } - Self::Aggregate { .. } => None, + DataTypeDeclaration::Aggregate { .. } => None, } } pub fn is_aggregate(&self) -> bool { - matches!(self, Self::Aggregate { .. }) + matches!(self, DataTypeDeclaration::Aggregate { .. }) } } diff --git a/src/codegen/debug.rs b/src/codegen/debug.rs index 2c8de8fdc2..c0bdd0b34e 100644 --- a/src/codegen/debug.rs +++ b/src/codegen/debug.rs @@ -534,7 +534,7 @@ impl<'ink> DebugBuilder<'ink> { } let implementation = pou.find_implementation(index).expect("A POU will have an impl at this stage"); - if !implementation.get_implementation_type().is_function_or_init() { + if !implementation.get_implementation_type().is_function_method_or_init() { self.register_struct_parameter(pou, func); } else { let declared_params = index.get_declared_parameters(implementation.get_call_name()); diff --git a/src/codegen/generators/pou_generator.rs b/src/codegen/generators/pou_generator.rs index 4a99c9a82e..4c7c58db0e 100644 --- a/src/codegen/generators/pou_generator.rs +++ b/src/codegen/generators/pou_generator.rs @@ -236,7 +236,7 @@ impl<'ink, 'cg> PouGenerator<'ink, 'cg> { } _ => { dti.map(|it| { - if !implementation.get_implementation_type().is_function_or_init() { + if !implementation.get_implementation_type().is_function_method_or_init() { return *p; } // for aggregate function parameters we will generate a pointer instead of the value type. @@ -354,7 +354,7 @@ impl<'ink, 'cg> PouGenerator<'ink, 'cg> { &self, implementation: &ImplementationIndexEntry, ) -> Result>, Diagnostic> { - if !implementation.implementation_type.is_function_or_init() { + if !implementation.implementation_type.is_function_method_or_init() { let mut parameters = vec![]; let instance_struct_type: StructType = self .llvm_index @@ -456,7 +456,7 @@ impl<'ink, 'cg> PouGenerator<'ink, 'cg> { } // generate local variables - if implementation.pou_type.is_function_or_init() { + if implementation.pou_type.is_function_method_or_init() { self.generate_local_function_arguments_accessors( &mut local_index, &implementation.type_name, diff --git a/src/codegen/generators/statement_generator.rs b/src/codegen/generators/statement_generator.rs index 4163b42b23..1db298bd09 100644 --- a/src/codegen/generators/statement_generator.rs +++ b/src/codegen/generators/statement_generator.rs @@ -45,7 +45,7 @@ pub struct StatementCodeGenerator<'a, 'b> { llvm: &'b Llvm<'a>, index: &'b Index, annotations: &'b AstAnnotations, - llvm_inex: &'b LlvmTypedIndex<'a>, + llvm_index: &'b LlvmTypedIndex<'a>, function_context: &'b FunctionContext<'a, 'b>, pub load_prefix: String, @@ -65,7 +65,7 @@ impl<'a, 'b> StatementCodeGenerator<'a, 'b> { llvm: &'b Llvm<'a>, index: &'b Index, annotations: &'b AstAnnotations, - llvm_inex: &'b LlvmTypedIndex<'a>, + llvm_index: &'b LlvmTypedIndex<'a>, linking_context: &'b FunctionContext<'a, 'b>, debug: &'b DebugBuilderEnum<'a>, ) -> StatementCodeGenerator<'a, 'b> { @@ -73,7 +73,7 @@ impl<'a, 'b> StatementCodeGenerator<'a, 'b> { llvm, index, annotations, - llvm_inex, + llvm_index, function_context: linking_context, load_prefix: "load_".to_string(), load_suffix: "".to_string(), @@ -100,7 +100,7 @@ impl<'a, 'b> StatementCodeGenerator<'a, 'b> { /// generates a list of statements pub fn generate_body(&self, statements: &[AstNode]) -> Result<(), Diagnostic> { - let mut child_index = LlvmTypedIndex::create_child(self.llvm_inex); + let mut child_index = LlvmTypedIndex::create_child(self.llvm_index); for s in statements { child_index = self.generate_statement(child_index, s)?; } @@ -783,7 +783,7 @@ impl<'a, 'b> StatementCodeGenerator<'a, 'b> { let var_name = format!("{call_name}_ret"); // TODO: Naming convention (see plc_util/src/convention.rs) let ret_name = ret_v.get_qualified_name(); let value_ptr = - self.llvm_inex.find_loaded_associated_variable_value(ret_name).ok_or_else(|| { + self.llvm_index.find_loaded_associated_variable_value(ret_name).ok_or_else(|| { Diagnostic::codegen_error( format!("Cannot generate return variable for {call_name:}"), SourceLocation::undefined(), diff --git a/src/codegen/tests/code_gen_tests.rs b/src/codegen/tests/code_gen_tests.rs index bb17d717fc..3293ffb12f 100644 --- a/src/codegen/tests/code_gen_tests.rs +++ b/src/codegen/tests/code_gen_tests.rs @@ -1006,7 +1006,6 @@ fn fb_method_with_var_input_defaults() { insta::assert_snapshot!(prg); } -//A test for a method with an initialized input variable #[test] fn method_codegen_with_initialized_input() { let prg = codegen( @@ -1027,7 +1026,6 @@ fn method_codegen_with_initialized_input() { insta::assert_snapshot!(prg); } -//A test for a method with multiple input variables #[test] fn method_codegen_with_multiple_input() { let prg = codegen( diff --git a/src/index.rs b/src/index.rs index 8660388bc6..849fbcc070 100644 --- a/src/index.rs +++ b/src/index.rs @@ -463,8 +463,7 @@ impl From<&PouType> for ImplementationType { } impl ImplementationType { - // TODO: this now also takes methods into accounts, find a better name - pub fn is_function_or_init(&self) -> bool { + pub fn is_function_method_or_init(&self) -> bool { matches!( self, ImplementationType::Function @@ -762,7 +761,6 @@ impl PouIndexEntry { match self { PouIndexEntry::Program { instance_struct_name, .. } | PouIndexEntry::FunctionBlock { instance_struct_name, .. } - // | PouIndexEntry::Method { instance_struct_name, .. } | PouIndexEntry::Action { instance_struct_name, .. } | PouIndexEntry::Class { instance_struct_name, .. } => Some(instance_struct_name.as_str()), _ => None, //functions have no struct type diff --git a/src/lowering/calls.rs b/src/lowering/calls.rs index 2f7b6224f5..f5c3634415 100644 --- a/src/lowering/calls.rs +++ b/src/lowering/calls.rs @@ -1,6 +1,49 @@ //! Changes the calls to aggregate return types //! to make them VAR_IN_OUT calls, allowing them //! to be called from C_APIs and simplifying code generation +//! +//! As a first step, the POU signature is changed. E.g. a function +//! returning a `STRING` will now return `__VOID` with the return variable +//! being moved into a `VAR_IN_OUT` block: +//! ```iec61131 +//! // user code +//! FUNCTION foo : STRING +//! VAR_INPUT +//! a: DINT; +//! END_VAR +//! END_FUNCTION +//! ``` +//! ```iec61131 +//! // lowered equivalent +//! FUNCTION foo +//! VAR_IN_OUT +//! foo: STRING; +//! END_VAR +//! VAR_INPUT +//! a: DINT; +//! END_VAR +//! END_FUNCTION +//! ``` +//! +//! Next, every call-statement to that POU has it's arguments updated, with a temporary +//! variable being allocated to hold the value. +//! Locally allocated variables follow a naming-scheme of `__`, +//! being a value from an atomically incremented counter to avoid naming conflicts +//! (the same approach is used for allocated variables in LLVM-IR). +//! ```iec61131 +//! // user code. Let `s` be a variable of type `STRING` +//! // ... +//! s := foo(42); +//! // ... +//! ``` +//! ```iec61131 +//! // lowered equivalent +//! // ... +//! alloca __foo1 : STRING; +//! foo(__foo1, 42); +//! s := __foo1; +//! // ... +//! ``` use std::{borrow::BorrowMut, sync::atomic::AtomicI32}; @@ -145,37 +188,43 @@ impl AstVisitorMut for AggregateTypeLowerer { return; } let index = self.index.as_ref().expect("Can't get here without an index"); - //Check if pou has a return type - if let Some(return_var) = pou.return_type.take() { - let name = return_var.get_name().expect("We should have names at this point"); - let location = return_var.get_location(); + + // Check if POU has a return type + let Some(return_type_name) = pou + .return_type + .as_ref() + .map(|it| it.get_name().expect("We should have names at this point").to_string()) + else { + return; + }; + + // If the return type is aggregate, remove it from the signature and add a matching variable + // in a VAR_IN_OUT block + if index.get_effective_type_or_void_by_name(&return_type_name).is_aggregate_type() { + let original_return = pou.return_type.take().unwrap(); + let location = original_return.get_location(); //Create a new return type for the pou pou.return_type.replace(plc_ast::ast::DataTypeDeclaration::Aggregate { - referenced_type: name.to_string(), + referenced_type: return_type_name, location, }); - let data_type = index.get_effective_type_or_void_by_name(name); - if data_type.is_aggregate_type() { - //Insert a new in out var to the pou variable block declarations - let block = VariableBlock { - access: AccessModifier::Public, - constant: false, - retain: false, - variables: vec![Variable { - name: pou.get_return_name().to_string(), - data_type_declaration: return_var, - initializer: None, - address: None, - location: pou.name_location.clone(), - }], - variable_block_type: VariableBlockType::InOut, - linkage: LinkageType::Internal, - location: SourceLocation::internal(), - }; - pou.variable_blocks.insert(0, block) - } else { - pou.return_type.replace(return_var); - } + //Insert a new in out var to the pou variable block declarations + let block = VariableBlock { + access: AccessModifier::Public, + constant: false, + retain: false, + variables: vec![Variable { + name: pou.get_return_name().to_string(), + data_type_declaration: original_return, + initializer: None, + address: None, + location: pou.name_location.clone(), + }], + variable_block_type: VariableBlockType::InOut, + linkage: LinkageType::Internal, + location: SourceLocation::internal(), + }; + pou.variable_blocks.insert(0, block) } } @@ -449,29 +498,6 @@ mod tests { assert_debug_snapshot!(lowerer.index.unwrap().find_pou_type("fb.complexMethod").unwrap()); } - // Are we in a call? - // foo(x:= baz()); callStatement -> Reference baz_1 - // foo(x:= baz()); callStatement -> Reference baz_2 - // foo(x:= baz()); callStatement -> Reference baz_3 - // foo(x:= baz()); callStatement -> Reference - // foo(x:= baz()); callStatement -> Reference - // foo(x:= baz()); callStatement -> Reference - // foo(x:= baz()); callStatement -> Reference - // alloca temp; - // baz(temp); - // foo(x := temp) - // - // call -> alloc, call, ref - // - // Insert alloca _before_ the call statement - // x := foo(); - // alloca temp - // foo(temp); - // x := temp; - //Check right, if a function call with aggregate, add allocation - //fix call - //assign to allocation - // #[test] fn simple_call_statement() { let id_provider = IdProvider::default(); diff --git a/src/resolver.rs b/src/resolver.rs index 3a1945e066..b933911ce5 100644 --- a/src/resolver.rs +++ b/src/resolver.rs @@ -6,7 +6,7 @@ //! records all resulting types associated with the statement's id. use rustc_hash::{FxHashMap, FxHashSet}; -use std::{any::Any, fmt::Debug, hash::Hash}; +use std::{fmt::Debug, hash::Hash}; use plc_ast::{ ast::{ @@ -554,7 +554,7 @@ impl Dependency { } } -pub trait AnnotationMap: ToAny { +pub trait AnnotationMap { fn get(&self, s: &AstNode) -> Option<&StatementAnnotation>; fn get_hint(&self, s: &AstNode) -> Option<&StatementAnnotation>; @@ -639,29 +639,6 @@ pub struct AstAnnotations { bool_annotation: StatementAnnotation, } -pub trait ToAny: 'static { - fn as_any(&mut self) -> &mut dyn Any; -} - -impl ToAny for T { - fn as_any(&mut self) -> &mut dyn Any { - self - } -} - -impl AstAnnotations { - pub fn from_dyn(mut annotation_map: Box, bool_id: AstId) -> Self { - let it: &mut dyn Any = annotation_map.as_any(); - if let Some(map) = it.downcast_mut::().map(std::mem::take) { - return map; - } - let annotation_map = - it.downcast_mut::().map(std::mem::take).expect("AnnotationMapImpl"); - - Self::new(annotation_map, bool_id) - } -} - impl AnnotationMap for AstAnnotations { fn get(&self, s: &AstNode) -> Option<&StatementAnnotation> { if s.get_id() == self.bool_id { diff --git a/src/validation/tests/pou_validation_tests.rs b/src/validation/tests/pou_validation_tests.rs index 69e5f224fe..4731556bcb 100644 --- a/src/validation/tests/pou_validation_tests.rs +++ b/src/validation/tests/pou_validation_tests.rs @@ -236,13 +236,13 @@ fn assigning_return_value_to_void_functions_returns_error() { } #[test] -fn method_input_arguments_are_not_optional() { +fn method_input_arguments_are_optional() { let diagnostic = parse_and_validate_buffered( " FUNCTION_BLOCK fb METHOD foo VAR_INPUT - in1 : BOOL; + in1 : BOOL := true; in2 : BOOL; END_VAR END_METHOD @@ -253,19 +253,16 @@ fn method_input_arguments_are_not_optional() { fbInstance : fb; END_VAR - // All of these are invalid because they are missing arguments + // All of these are valid because parameters will default to their initial values if not given + // explicit arguments fbInstance.foo(); fbInstance.foo(in1 := TRUE); fbInstance.foo(in2 := TRUE); - - // These are valid - fbInstance.foo(in1 := TRUE, in2 := TRUE); - fbInstance.foo(in2 := TRUE, in1 := TRUE); END_FUNCTION ", ); - assert_snapshot!(diagnostic, @""); + assert!(diagnostic.is_empty()) } #[test]