Skip to content

Conversation

@omsherikar
Copy link
Contributor

Pull Request description

Implements LLVM IR codegen for astx.StructDefStmt to generate IdentifiedStructType definitions. Adds struct tracking dictionaries (struct_types and struct_defs) to track struct definitions for future use in struct instantiation and field access.

Fixes #143

How to test these changes

  • Create a StructDefStmt with attributes (e.g., Point with x, y as Int32)
  • Translate to LLVM IR using LLVMLiteIR().translator.translate(module)
  • Verify struct definition appears in output as %"Point" = type {i32, i32}

Pull Request checklists

This PR is a:

  • bug-fix
  • new feature
  • maintenance

About this PR:

  • it includes tests.
  • the tests are executed on CI.
  • the tests generate log file(s) (path).
  • pre-commit hooks were executed locally.
  • this PR requires a project documentation update.

Author's checklist:

  • I have reviewed the changes and it contains no misspelling.
  • The code is well commented, especially in the parts that contain more
    complexity.
  • New and old tests passed locally.

Additional information

Example LLVM IR output:vm
%"Point" = type {i32, i32}## Reviewer's checklist

Copy and paste this template for your review's note:

Copilot AI review requested due to automatic review settings December 18, 2025 17:58
@github-actions
Copy link

OSL ChatGPT Reviewer

NOTE: This is generated by an AI program, so some comments may not make sense.

src/irx/builders/llvmliteir.py

  • Potentially incorrect type resolution: deriving field types from attr.type_.class.name.lower() is brittle and will mis-handle user-defined structs, pointers, arrays, and aliases. Replace with a proper ASTx->IR type resolver, and support forward-declared/named structs via context.get_identified_type (L.1932).
    Suggested change:
    def _ir_type_from_ast_type(self, t: astx.Type, current_struct: str | None = None) -> ir.Type:
    """Resolve ASTx type to LLVM IR type."""
    ...
    And in visit():
    field_type = self.ir_type_from_ast_type(attr.type, current_struct=node.name)

  • Recursive/self-referential structs: current code allows by-value self fields, which LLVM disallows for recursive types. Detect when a field references the enclosing struct and force a pointer type or raise (L.1932).
    Suggested guard inside the resolver:
    def _ir_type_from_ast_type(self, t: astx.Type, current_struct: str | None = None) -> ir.Type:
    """Resolve ASTx type to LLVM IR type."""
    # if t is a named type equal to current_struct:
    # return ir.PointerType(self._llvm.module.context.get_identified_type(current_struct))
    # else resolve normally

  • Redefinition guard is incomplete: if an identified type with the same name already exists in the LLVM context (created elsewhere) but not tracked in struct_types, this will silently reuse it. Add a check after get_identified_type to detect non-opaque types and raise to avoid accidental redefinition (L.1926).
    Suggested change:
    def _ensure_fresh_struct(self, struct_type: ir.IdentifiedStructType, name: str) -> None:
    """Ensure struct is not already defined."""
    if not struct_type.is_opaque and name not in self.struct_types:
    raise Exception(f"Struct '{name}' already defined in LLVM context.")

  • Forward references to other structs: enable referencing other structs not yet defined by obtaining identified types by name (and using pointers where appropriate), rather than relying on get_data_type’s primitive mapping (L.1932).


Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements LLVM IR code generation for astx.StructDefStmt to enable struct type definitions in the IRX compiler. The changes add infrastructure to track struct types and their definitions for future use in struct instantiation and field access operations.

Key Changes:

  • Added struct tracking dictionaries (struct_types and struct_defs) to the LLVMLiteIRVisitor class
  • Implemented visit(StructDefStmt) method to generate LLVM IdentifiedStructType definitions
  • Converts ASTx struct attributes to LLVM field types using existing type mapping infrastructure

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

field_types = []
for attr in node.attributes:
type_str = attr.type_.__class__.__name__.lower()
field_type = self._llvm.get_data_type(type_str)
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The type conversion logic using attr.type_.class.name.lower() may fail if get_data_type doesn't recognize the type name. When get_data_type encounters an unknown type at line 145, it raises an exception with "[EE]: Type name {type_name} not valid." However, this could be misleading for struct field types. Consider adding a try-except block or pre-validating the type to provide a more specific error message indicating which field in which struct has an unsupported type.

Suggested change
field_type = self._llvm.get_data_type(type_str)
try:
field_type = self._llvm.get_data_type(type_str)
except Exception as e:
# Provide more context for unsupported struct field types.
field_name = getattr(attr, "name", "<unknown>")
raise Exception(
f"Unsupported type '{type_str}' for field '{field_name}' "
f"in struct '{node.name}'."
) from e

Copilot uses AI. Check for mistakes.
field_type = self._llvm.get_data_type(type_str)
field_types.append(field_type)

struct_type.set_body(*field_types)
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The implementation doesn't handle the case where node.attributes is empty. Creating a struct with no fields will result in calling struct_type.set_body() with no arguments, which may not be the intended behavior for LLVM IR. Consider adding a check to handle empty structs or document this behavior if it's intentional.

Suggested change
struct_type.set_body(*field_types)
if field_types:
# Struct with one or more fields.
struct_type.set_body(*field_types)
else:
# Intentionally define an empty struct when there are no attributes.
struct_type.set_body()

Copilot uses AI. Check for mistakes.
field_type = self._llvm.get_data_type(type_str)
field_types.append(field_type)

struct_type.set_body(*field_types)
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The visit method for StructDefStmt does not append to result_stack. Other similar visitor methods (like visit for FunctionDef at line 1919 and FunctionPrototype at line 1958) append their results to result_stack. This could cause issues if code expects to retrieve the struct type from the result stack. Consider appending struct_type to result_stack for consistency.

Suggested change
struct_type.set_body(*field_types)
struct_type.set_body(*field_types)
self.result_stack.append(struct_type)

Copilot uses AI. Check for mistakes.
def visit(self, node: astx.StructDefStmt) -> None:
"""Translate ASTx StructDefStmt to LLVM-IR."""
if node.name in self.struct_types:
raise Exception(f"Struct '{node.name}' already defined.")
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The error message format is inconsistent with other error messages in the codebase. Most error messages use a prefix like "[EE]:" (see line 145) or "codegen:" (see lines 500, 507, 512, etc.). Consider using a consistent prefix format such as "codegen: Struct 'name' already defined." for better error tracking and consistency.

Suggested change
raise Exception(f"Struct '{node.name}' already defined.")
raise Exception(f"codegen: Struct '{node.name}' already defined.")

Copilot uses AI. Check for mistakes.
@omsherikar
Copy link
Contributor Author

omsherikar commented Dec 18, 2025

@xmnlab @yuvimittal please have a look

@omsherikar
Copy link
Contributor Author

@xmnlab @yuvimittal can you please look into it?

@github-actions
Copy link

OSL ChatGPT Reviewer

NOTE: This is generated by an AI program, so some comments may not make sense.

src/irx/builders/llvmliteir.py

ChatGPT was not able to review the file. Error: Error code: 429 - {'error': {'message': 'You exceeded your current quota, please check your plan and billing details. For more information on this error, read the docs: https://platform.openai.com/docs/guides/error-codes/api-errors.&#x27;, 'type': 'insufficient_quota', 'param': None, 'code': 'insufficient_quota'}}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add LLVM IR codegen for StructDefStmt (struct type definitions)

1 participant