Skip to content

fix: extended function block body calls #1545

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix: extended function block body calls #1545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6cba4ce
fix: function block calls with inherited parameters
volsa Nov 4, 2025
cb65351
refactor: self-review
volsa Nov 12, 2025
70a80f3
Merge branch 'master' into vosa/PRG-3463
volsa Nov 13, 2025
1793c9f
docs: add code comment explaining GEP logic
volsa Nov 14, 2025
67867b3
Merge branch 'master' into vosa/PRG-3463
volsa Nov 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions compiler/plc_ast/src/ast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1484,6 +1484,16 @@ impl AstNode {
pub fn is_real(&self) -> bool {
matches!(self.stmt, AstStatement::Literal(AstLiteral::Real(_), ..))
}

/// Returns the identifier of the left-hand side if this is an assignment statement
pub fn get_assignment_identifier(&self) -> Option<&str> {
match &self.stmt {
AstStatement::Assignment(Assignment { left, .. })
| AstStatement::OutputAssignment(Assignment { left, .. })
| AstStatement::RefAssignment(Assignment { left, .. }) => left.get_flat_reference_name(),
_ => None,
}
}
}

#[derive(Clone, Copy, Debug, PartialEq, Eq)]
Expand Down
4 changes: 2 additions & 2 deletions src/builtins.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,7 +672,7 @@ fn annotate_comparison_function(
.has_nature(TypeNature::Elementary, annotator.index)
}) {
// we are trying to call this function with a non-elementary type, so we redirect back to the resolver
annotator.annotate_call_statement(operator, Some(parameters), &ctx);
annotator.annotate_arguments(operator, parameters, &ctx);
return;
}

Expand Down Expand Up @@ -741,7 +741,7 @@ fn annotate_arithmetic_function(
.has_nature(TypeNature::Num, annotator.index)
}) {
// we are trying to call this function with a non-numerical type, so we redirect back to the resolver
annotator.annotate_call_statement(operator, Some(parameters), &ctx);
annotator.annotate_arguments(operator, parameters, &ctx);
return;
}

Expand Down
147 changes: 91 additions & 56 deletions src/codegen/generators/expression_generator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,25 @@ pub struct ExpressionCodeGenerator<'a, 'b> {
/// context information to generate a parameter
#[derive(Debug)]
struct CallParameterAssignment<'a, 'b> {
/// the assignmentstatement in the call-argument list (a:=3)
/// The named argument node of the function call (e.g. `foo(in := 3)`)
assignment: &'b AstNode,
/// the name of the function we're calling

/// The name of the POU being called
function_name: &'b str,
/// the position of the argument in the POU's argument's list
index: u32,
/// a pointer to the struct instance that carries the call's arguments

/// The position of the parameter in the POUs variable declaration list.
/// See also [`StatementAnnotation::Argument::position`].
position: u32,

/// The depth between where the parameter is declared versus where it is being called from.
/// See also [`StatementAnnotation::Argument::depth`].
depth: u32,

/// The name of the POU where the parameter is declared
/// See also [`StatementAnnotation::Argument::pou`].
pou_parameter: &'b str,
Copy link
Member

@mhasel mhasel Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it weren't for the doc-comment, I'd have no idea that pou_parameter refers to the name of the POU. How do you feel about something like declaring_pou or source_pou?


/// The pointer to the struct instance that carries the call's arguments
parameter_struct: PointerValue<'a>,
}

Expand Down Expand Up @@ -702,14 +714,18 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {
let is_output = pou_info.get(index).is_some_and(|param| param.get_variable_type().is_output());

if assignment_statement.is_output_assignment() || (implicit && is_output) {
let Some(StatementAnnotation::Argument { position, depth, pou, .. }) =
self.annotations.get_hint(assignment_statement)
else {
panic!()
Copy link
Member

@mhasel mhasel Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we return an error here instead of an explicit panic? If we are confident this code-path can't be reached, we might wanna change this to unreachable instead

};

self.assign_output_value(&CallParameterAssignment {
assignment: assignment_statement,
function_name,
index: self
.annotations
.get_hint(assignment_statement)
.and_then(StatementAnnotation::get_location_in_parent)
.expect("arguments must have a type hint"),
position: *position as u32,
depth: *depth as u32,
pou_parameter: pou,
parameter_struct,
})?
}
Expand All @@ -720,11 +736,9 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {

fn assign_output_value(&self, param_context: &CallParameterAssignment) -> Result<(), CodegenError> {
match &param_context.assignment.stmt {
AstStatement::OutputAssignment(assignment) => self.generate_explicit_output_assignment(
param_context.parameter_struct,
param_context.function_name,
assignment,
),
AstStatement::OutputAssignment(assignment) => {
self.generate_explicit_output_assignment(param_context, assignment)
}

_ => self.generate_output_assignment(param_context),
}
Expand Down Expand Up @@ -839,16 +853,44 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {
Ok(())
}

fn build_parameter_struct_gep(&self, context: &CallParameterAssignment<'ink, 'b>) -> PointerValue<'ink> {
unsafe {
// Assuming we have an inheritance hierarchy of `A <- B <- C` and a call `objC(inA := 1, ...)`
// then in order to access `inA` in `C` we have to generate some IR of form `objC.__B.__A.inA`.
// Because the parent structs are always the first member of these function blocks / classes, we
// can simply generate a GEP with indices `0` and repeat that `depth` times followed by the
// actual position of the parameter. So `objC.__B.__A.inA` becomes `GEP objC, 0, 0, 0, <inA pos>`
let mut gep_index = vec![0; (context.depth + 1) as usize];
gep_index.push(context.position);

let i32_type = self.llvm.context.i32_type();
let gep_index = gep_index
.into_iter()
.map(|value| i32_type.const_int(value as u64, false))
.collect::<Vec<_>>();

self.llvm.builder.build_gep(context.parameter_struct, &gep_index, "").unwrap()
Copy link
Member

@mhasel mhasel Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you tell me why we aren't using build_in_bounds_gep here? In your PR message you mention

GEPing directly, i.e. not inbound

An inbound GEP is still a direct GEP, just with additional safety guarantees and it allows LLVM to optimize more aggressively. Inheritance hierarchy access is well defined and guaranteed to be within bounds of the allocated struct, or am I missing something?
I have changed this line to use build_in_bounds_gep and the LIT tests still pass locally

}
}

fn generate_output_assignment(&self, context: &CallParameterAssignment) -> Result<(), CodegenError> {
let &CallParameterAssignment { assignment: expr, function_name, index, parameter_struct } = context;
let &CallParameterAssignment {
assignment: expr,
function_name,
position: index,
depth: _,
pou_parameter: arg_pou,
parameter_struct,
} = context;

let builder = &self.llvm.builder;

// We don't want to generate any code if the right side of an assignment is empty, e.g. `foo(out =>)`
if expr.is_empty_statement() {
return Ok(());
}

let parameter = self.index.get_declared_parameter(function_name, index).expect("must exist");
let parameter = self.index.get_declared_parameter(arg_pou, index).expect("must exist");

match expr.get_stmt() {
AstStatement::ReferenceExpr(_) if expr.has_direct_access() => {
Expand Down Expand Up @@ -889,12 +931,7 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {
let assigned_output = self.generate_lvalue(expr)?;
let assigned_output_type =
self.annotations.get_type_or_void(expr, self.index).get_type_information();
let output = builder.build_struct_gep(parameter_struct, index, "").map_err(|_| {
Diagnostic::codegen_error(
format!("Cannot build generate parameter: {parameter:#?}"),
&parameter.source_location,
)
})?;
let output = self.build_parameter_struct_gep(context);

let output_value_type = self.index.get_type_information_or_void(parameter.get_type_name());

Expand All @@ -919,24 +956,21 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {

fn generate_explicit_output_assignment(
&self,
parameter_struct: PointerValue<'ink>,
function_name: &str,
param_context: &CallParameterAssignment,
assignment: &Assignment,
) -> Result<(), CodegenError> {
let parameter_struct = param_context.parameter_struct;
let function_name = param_context.function_name;
let Assignment { left, right } = assignment;

if let Some(StatementAnnotation::Variable { qualified_name, .. }) = self.annotations.get(left) {
let parameter = self
.index
.find_fully_qualified_variable(qualified_name)
.ok_or_else(|| Diagnostic::unresolved_reference(qualified_name, left.as_ref()))?;
let index = parameter.get_location_in_parent();

if let Some(StatementAnnotation::Variable { .. }) = self.annotations.get(left) {
self.generate_output_assignment(&CallParameterAssignment {
assignment: right,
function_name,
index,
position: param_context.position,
depth: param_context.depth,
parameter_struct,
pou_parameter: param_context.pou_parameter,
})?
};

Expand Down Expand Up @@ -1280,14 +1314,18 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {
})
.unwrap_or_else(|| vec![parameter_struct.as_basic_value_enum().into()]);
for argument in arguments.iter() {
let Some(StatementAnnotation::Argument { position, depth, pou, .. }) =
self.annotations.get_hint(argument)
else {
panic!()
};

let parameter = self.generate_call_struct_argument_assignment(&CallParameterAssignment {
assignment: argument,
function_name: pou_name,
index: self
.annotations
.get_hint(argument)
.and_then(StatementAnnotation::get_location_in_parent)
.expect("arguments must have a type hint"),
position: *position as u32,
depth: *depth as u32,
pou_parameter: pou,
parameter_struct,
})?;
if let Some(parameter) = parameter {
Expand Down Expand Up @@ -1378,28 +1416,23 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {
param_context: &CallParameterAssignment,
) -> Result<Option<BasicValueEnum<'ink>>, CodegenError> {
let builder = &self.llvm.builder;
let function_name = param_context.function_name;
let index = param_context.index;
let parameter_struct = param_context.parameter_struct;
// let function_name = param_context.function_name;
Copy link
Member

@mhasel mhasel Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed a comment

let index = param_context.position;
let expression = param_context.assignment;
if let Some(parameter) = self.index.get_declared_parameter(function_name, index) {

if let Some(parameter) = self.index.get_declared_parameter(param_context.pou_parameter, index) {
// this happens before the pou call
// before the call statement we may only consider inputs and inouts
// after the call we need to copy the output values to the correct assigned variables
if matches!(parameter.get_variable_type(), VariableType::Output) {
return Ok(None);
}

let pointer_to_param = builder.build_struct_gep(parameter_struct, index, "").map_err(|_| {
Diagnostic::codegen_error(
format!("Cannot build generate parameter: {expression:#?}"),
expression,
)
})?;
let pointer_to_param = self.build_parameter_struct_gep(param_context);

let parameter = self
.index
.find_parameter(function_name, index)
.find_parameter(param_context.pou_parameter, index)
.and_then(|var| self.index.find_effective_type_by_name(var.get_type_name()))
.map(|var| var.get_type_information())
.unwrap_or_else(|| self.index.get_void_type().get_type_information());
Expand Down Expand Up @@ -1444,21 +1477,23 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {
.index
.find_fully_qualified_variable(qualified_name)
.ok_or_else(|| Diagnostic::unresolved_reference(qualified_name, left))?;
let index = parameter.get_location_in_parent();

// don't generate param assignments for empty statements, with the exception
// of VAR_IN_OUT params - they need an address to point to
let is_auto_deref = self
.index
.find_effective_type_by_name(parameter.get_type_name())
.map(|var| var.get_type_information())
.unwrap_or(self.index.get_void_type().get_type_information())
.map(DataType::get_type_information)
.unwrap_or_else(|| self.index.get_void_type().get_type_information())
.is_auto_deref();

if !right.is_empty_statement() || is_auto_deref {
self.generate_call_struct_argument_assignment(&CallParameterAssignment {
assignment: right,
function_name,
index,
position: param_context.position,
depth: param_context.depth,
pou_parameter: param_context.pou_parameter,
parameter_struct,
})?;
};
Expand Down Expand Up @@ -1504,7 +1539,7 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {
let member_location = self
.index
.find_fully_qualified_variable(qualified_name)
.map(VariableIndexEntry::get_location_in_parent)
.map(VariableIndexEntry::get_position)
.ok_or_else(|| Diagnostic::unresolved_reference(qualified_name, offset))?;
let gep: PointerValue<'_> =
self.llvm.get_member_pointer_from_struct(*qualifier, member_location, name)?;
Expand Down Expand Up @@ -2246,7 +2281,7 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {
Diagnostic::unresolved_reference(qualified_name, data.left.as_ref())
})?;

let index_in_parent = member.get_location_in_parent();
let index_in_parent = member.get_position();
let value = self.generate_expression(data.right.as_ref())?;

uninitialized_members.remove(member);
Expand Down Expand Up @@ -2278,7 +2313,7 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {
Diagnostic::cannot_generate_initializer(member.get_qualified_name(), assignments)
})?;

member_values.push((member.get_location_in_parent(), initial_value));
member_values.push((member.get_position(), initial_value));
}
let struct_type = self.llvm_index.get_associated_type(struct_name)?.into_struct_type();
if member_values.len() == struct_type.count_fields() as usize {
Expand Down
8 changes: 4 additions & 4 deletions src/codegen/tests/directaccess_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ fn direct_acess_in_output_assignment_implicit_explicit_and_mixed() {
%0 = bitcast %FOO* %f to i8*
call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 1 %0, i8* align 1 getelementptr inbounds (%FOO, %FOO* @__FOO__init, i32 0, i32 0), i64 ptrtoint (%FOO* getelementptr (%FOO, %FOO* null, i32 1) to i64), i1 false)
store i32 0, i32* %main, align 4
%1 = getelementptr inbounds %FOO, %FOO* %f, i32 0, i32 0
%1 = getelementptr %FOO, %FOO* %f, i32 0, i32 0
%load_error_bits = load i8, i8* %error_bits, align 1
%shift = lshr i8 %load_error_bits, 0
%2 = and i8 %shift, 1
Expand All @@ -182,7 +182,7 @@ fn direct_acess_in_output_assignment_implicit_explicit_and_mixed() {
%value = shl i8 %5, 0
%or = or i8 %erase, %value
store i8 %or, i8* %error_bits, align 1
%6 = getelementptr inbounds %FOO, %FOO* %f, i32 0, i32 0
%6 = getelementptr %FOO, %FOO* %f, i32 0, i32 0
%load_error_bits1 = load i8, i8* %error_bits, align 1
%shift2 = lshr i8 %load_error_bits1, 0
%7 = and i8 %shift2, 1
Expand All @@ -195,7 +195,7 @@ fn direct_acess_in_output_assignment_implicit_explicit_and_mixed() {
%value4 = shl i8 %10, 0
%or5 = or i8 %erase3, %value4
store i8 %or5, i8* %error_bits, align 1
%11 = getelementptr inbounds %FOO, %FOO* %f, i32 0, i32 0
%11 = getelementptr %FOO, %FOO* %f, i32 0, i32 0
%load_error_bits6 = load i8, i8* %error_bits, align 1
%shift7 = lshr i8 %load_error_bits6, 0
%12 = and i8 %shift7, 1
Expand All @@ -208,7 +208,7 @@ fn direct_acess_in_output_assignment_implicit_explicit_and_mixed() {
%value9 = shl i8 %15, 0
%or10 = or i8 %erase8, %value9
store i8 %or10, i8* %error_bits, align 1
%16 = getelementptr inbounds %FOO, %FOO* %f, i32 0, i32 0
%16 = getelementptr %FOO, %FOO* %f, i32 0, i32 0
%load_error_bits11 = load i8, i8* %error_bits, align 1
%shift12 = lshr i8 %load_error_bits11, 0
%17 = and i8 %shift12, 1
Expand Down
6 changes: 3 additions & 3 deletions src/codegen/tests/fnptr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,13 +412,13 @@ fn function_block_body() {
store i32 0, i32* %localOut, align 4
store i64 0, i64* %localInout, align 8
%1 = load void (%A*)*, void (%A*)** %bodyPtr, align 8
%2 = getelementptr inbounds %A, %A* %instanceA, i32 0, i32 1
%2 = getelementptr %A, %A* %instanceA, i32 0, i32 1
%load_localIn = load i16, i16* %localIn, align 2
store i16 %load_localIn, i16* %2, align 2
%3 = getelementptr inbounds %A, %A* %instanceA, i32 0, i32 3
%3 = getelementptr %A, %A* %instanceA, i32 0, i32 3
store i64* %localInout, i64** %3, align 8
call void %1(%A* %instanceA)
%4 = getelementptr inbounds %A, %A* %instanceA, i32 0, i32 2
%4 = getelementptr %A, %A* %instanceA, i32 0, i32 2
%5 = load i32, i32* %4, align 4
store i32 %5, i32* %localOut, align 4
ret void
Expand Down
Loading
Loading