Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces experimental MLIR (Multi-Level Intermediate Representation) support to the Ora compiler, enabling the lowering of Ora source code to MLIR IR through a modular architecture. The implementation includes comprehensive build system integration, a new CLI command for MLIR emission, and a well-structured codebase organized into specialized modules for different aspects of MLIR lowering.
Key changes include:
- Added modular MLIR lowering system with separate files for types, expressions, statements, declarations, memory management, symbols, and locations
- Integrated MLIR libraries into the build system with CMake-based compilation from a vendored LLVM submodule
- Added
mlirCLI subcommand and demo executable for testing MLIR functionality
Reviewed Changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/mlir/types.zig |
Comprehensive type mapping system for converting Ora types to MLIR types with support for all primitive and complex types |
src/mlir/symbols.zig |
Symbol table and parameter mapping infrastructure for scope management and variable tracking |
src/mlir/statements.zig |
Statement lowering system for converting Ora statements to MLIR operations including control flow |
src/mlir/mod.zig |
Main module file that exports all MLIR components and provides convenient type aliases |
src/mlir/memory.zig |
Memory region management for different Ora storage types (storage, memory, tstore, stack) |
src/mlir/lower.zig |
Core lowering logic with extensive duplication that should be refactored to use the modular components |
src/mlir/locations.zig |
Location tracking system for preserving source information in MLIR operations |
src/mlir/expressions.zig |
Expression lowering system for arithmetic, comparison, logical, and bitwise operations |
src/mlir/emit.zig |
MLIR module emission utilities for writing to files |
src/mlir/dialect.zig |
Ora dialect operations and helper functions for MLIR operation creation |
src/mlir/declarations.zig |
Declaration lowering for functions, contracts, structs, enums, and global variables |
src/mlir/context.zig |
MLIR context management with dialect registration |
src/mlir/c.zig |
C bindings for MLIR API access |
src/main.zig |
Added mlir subcommand for experimental MLIR emission |
ora-example/smoke.ora |
Simplified contract example for testing MLIR functionality |
examples/demos/mlir_demo.zig |
Demo executable showing Ora to MLIR conversion pipeline |
build.zig |
Added MLIR library building and linking with CMake integration and Ninja support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/mlir/lower.zig
Outdated
| @@ -0,0 +1,1540 @@ | |||
| // TODO: This file contains duplicated code that should be moved to modular files | |||
There was a problem hiding this comment.
This file contains extensive code duplication with the modular MLIR files. The TODO comments indicate that ParamMap, LocalVarMap, StorageMap, and various lowering functions should be moved to their respective modules (symbols.zig, memory.zig, expressions.zig, statements.zig, declarations.zig). This creates maintenance burden and potential inconsistencies. Consider refactoring this file to use the modular components instead of duplicating the implementation.
| pub fn toMlirType(self: *const TypeMapper, ora_type: anytype) c.MlirType { | ||
| if (ora_type.ora_type) |ora_ty| { |
There was a problem hiding this comment.
The parameter ora_type uses anytype which makes the API unclear and error-prone. Consider using a specific type like lib.ast.type_info.OraType or creating a union type that encompasses all valid input types. This would provide better type safety and clearer API documentation.
| pub fn toMlirType(self: *const TypeMapper, ora_type: anytype) c.MlirType { | |
| if (ora_type.ora_type) |ora_ty| { | |
| pub fn toMlirType(self: *const TypeMapper, ora_type: *const lib.ast.type_info.OraType) c.MlirType { | |
| if (ora_type) |ora_ty| { |
src/mlir/types.zig
Outdated
| // Default to i256 for unknown types | ||
| return c.mlirIntegerTypeGet(self.ctx, 256); |
There was a problem hiding this comment.
Defaulting to i256 for unknown types could mask type errors and lead to incorrect behavior. Consider returning an error or using a more explicit approach when the type cannot be determined, such as a Result type or explicit error handling.
| // Default to i256 for unknown types | |
| return c.mlirIntegerTypeGet(self.ctx, 256); | |
| // Return error for unknown types instead of defaulting to i256 | |
| return Error.UnknownType; |
src/mlir/expressions.zig
Outdated
| }, | ||
| else => { | ||
| // For other literal types, return a default value | ||
| const ty = c.mlirIntegerTypeGet(self.ctx, 256); |
There was a problem hiding this comment.
The literal value 256 appears multiple times throughout the codebase for bit width. Consider defining a named constant like DEFAULT_INTEGER_BITS = 256 to improve maintainability and make the intent clearer.
| const ty = c.mlirIntegerTypeGet(self.ctx, 256); | |
| const ty = c.mlirIntegerTypeGet(self.ctx, DEFAULT_INTEGER_BITS); |
src/mlir/statements.zig
Outdated
|
|
||
| /// Lower block body by processing all statements | ||
| pub fn lowerBlockBody(self: *const StatementLowerer, b: lib.ast.Statements.BlockNode, block: c.MlirBlock) void { | ||
| std.debug.print("DEBUG: Processing block with {d} statements\n", .{b.statements.len}); |
There was a problem hiding this comment.
Debug print statements should be removed or replaced with proper logging before production. These statements will clutter the output and may impact performance.
| std.debug.print("DEBUG: Processing block with {d} statements\n", .{b.statements.len}); | |
| // Removed debug print: Processing block with {d} statements |
src/mlir/memory.zig
Outdated
| .Stack => { | ||
| // For stack variables, we return the value directly from our local variable map | ||
| // This is handled differently in the identifier lowering | ||
| @panic("Stack variables should not use createLoadOperation"); |
There was a problem hiding this comment.
Using @panic for invalid input handling is not ideal for API design. Consider returning an error type instead to allow callers to handle this case gracefully. This would make the API more robust and testable.
| @panic("Stack variables should not use createLoadOperation"); | |
| return error.InvalidStorageType; |
src/mlir/lower.zig
Outdated
| // For now, panic on complex callee expressions | ||
| std.debug.print("DEBUG: Unhandled callee type: {s}\n", .{@tagName(call.callee.*)}); | ||
| @panic("Complex callee expressions not yet supported"); |
There was a problem hiding this comment.
Using @panic for unimplemented features can cause unexpected crashes. Consider returning an error or creating a placeholder implementation that logs the limitation and returns a reasonable default value.
| // For now, panic on complex callee expressions | |
| std.debug.print("DEBUG: Unhandled callee type: {s}\n", .{@tagName(call.callee.*)}); | |
| @panic("Complex callee expressions not yet supported"); | |
| // For now, log and return a default value for complex callee expressions | |
| std.debug.print("DEBUG: Unhandled callee type: {s}\n", .{@tagName(call.callee.*)}); | |
| std.debug.print("WARNING: Complex callee expressions not yet supported. Returning default value.\n", .{}); | |
| return c.MlirValue{}; |
src/mlir/symbols.zig
Outdated
| scope_idx -= 1; | ||
| } | ||
| // If symbol not found, add it to current scope | ||
| try self.addSymbol(name, c.mlirValueGetType(value), "stack", null); |
There was a problem hiding this comment.
The hardcoded string "stack" should be replaced with a proper enum value or constant to maintain consistency with the MemoryRegion enum used elsewhere in the codebase.
| try self.addSymbol(name, c.mlirValueGetType(value), "stack", null); | |
| try self.addSymbol(name, c.mlirValueGetType(value), lib.ast.Statements.MemoryRegion.Stack, null); |
This pull request adds experimental support for MLIR (Multi-Level Intermediate Representation) to the build system and compiler frontend, enabling Ora source files to be lowered to MLIR IR. The changes include new build steps for MLIR, integration of MLIR libraries, a demo executable, and a prototype command for MLIR emission. Additionally, a simplified example contract is provided for testing.
MLIR Integration and Build System Enhancements:
buildMlirLibrariesandlinkMlirLibrariestobuild.zigto build MLIR from a vendored LLVM submodule and link it to executables, including platform-specific flags and Ninja build support for faster builds. [1] [2] [3]mlir_demo) and build step, which runs a sample pipeline from Ora source to MLIR IR.src/mlir/c.zigand context management insrc/mlir/context.zig. [1] [2]Compiler Frontend and CLI Improvements:
mlirsubcommand insrc/main.zigto run the frontend pipeline and emit MLIR, with error handling and output to stdout. Updated usage documentation accordingly. [1] [2] [3]Demo and Example Updates:
examples/demos/mlir_demo.zigas a demo for lowering Ora to MLIR, including lexing, parsing, and MLIR module emission.ora-example/smoke.oracontract for easier testing with the new MLIR pipeline.