Attach custom diagnostic server in JIT and expander #44
Conversation
|
Warning Rate limit exceeded@jackalcooper has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces significant modifications across multiple modules in the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (14)
lib/charms/term.ex (1)
8-8: Document the purpose of the new _params parameter.While the signature change aligns with similar updates across other modules, it would be helpful to document the intended purpose of the
_paramsparameter, even if it's currently unused. This will help maintain clarity for future development.Consider adding a
@docstring or a comment explaining the role of this parameter in the context of intrinsic handling.lib/charms/env.ex (1)
8-8: Consider documenting the purpose of _paramsWhile the unused parameter follows Elixir conventions with the
_prefix, it would be helpful to document why this parameter exists, especially since it's part of a behavior implementation.@impl true + # params is unused but required for consistency with other intrinsic implementations def handle_intrinsic(:t, _params, [], opts) dolib/charms/simd.ex (1)
Line range hint
10-23: Consider using CompileError instead of ArgumentErrorThe function raises
ArgumentErrorfor validation, but according to the PR summary, there's a shift towards usingCompileErrorwith the new diagnostic server for compile-time error reporting. Consider updating the error handling to maintain consistency with this approach.Here's the suggested change:
if Enum.count(values) != width do - raise ArgumentError, "expected #{width} values, got #{length(values)}" + raise CompileError, description: "expected #{width} values, got #{length(values)}" endlib/charms/diagnostic.ex (1)
1-3: Consider adding internal documentation.While this is an internal module (
@moduledoc false), adding some documentation about its role in the diagnostic system would help future maintainers understand its purpose and integration with the Beaver diagnostic server.defmodule Charms.Diagnostic do @moduledoc false + # Internal module for handling compilation diagnostics and error reporting. + # Integrates with Beaver.Diagnostic.Server for structured error messages. @doc falselib/charms/intrinsic.ex (1)
Line range hint
1-19: Documentation needs updating to reflect the new callback signature.The module and callback documentation should be updated to explain:
- The purpose of the new
[Macro.t()]parameter- How it relates to diagnostic capabilities
- Update the "More on higher-order intrinsic" section
Here's a suggested update for the callback documentation:
@doc """ Callback to implement an intrinsic. + The callback accepts: + - name: The intrinsic function name as an atom + - args: The macro expressions of the arguments for diagnostic purposes + - params: The evaluated argument terms + - opts: Additional options including context, block, and location + Having different return types, there are two kinds of intrinsic functions: - Regular: returns a MLIR value or operation. - Higher-order: returns a function that returns a MLIR value or operation. ## More on higher-order intrinsic Higher-order intrinsic function can be variadic, which means it a list will be passed as arguments. + The macro expressions of arguments are provided separately from the evaluated terms to enable + better error diagnostics during compilation. """lib/charms/prelude.ex (1)
Line range hint
90-105: Enhance error reporting for ENIF function calls.Since this PR focuses on diagnostic improvements, consider enhancing error reporting for ENIF calls:
- Validate argument types match the ENIF signature
- Use the provided
locparameter for detailed error messages- Integrate with the diagnostic server for compile-time errors
def handle_intrinsic(name, _params, args, opts) when name in @enif_functions do {arg_types, ret_types} = Beaver.ENIF.signature(opts[:ctx], name) + # Validate argument count + unless length(args) == length(arg_types) do + raise CompileError, + description: "Invalid number of arguments for #{name}: expected #{length(arg_types)}, got #{length(args)}", + location: opts[:loc] + end args = args |> Enum.zip(arg_types) |> Enum.map(&wrap_arg(&1, opts))test/defm_test.exs (2)
112-122: LGTM with a minor suggestionThe test case effectively verifies the compile-time detection of undefined functions. Consider making the test description more specific by renaming it to "raises compile error for undefined remote function" to better reflect its purpose.
- test "undefined remote function" do + test "raises compile error for undefined remote function" do
140-153: Consider expanding diagnostic test coverageWhile the current test effectively verifies basic return type mismatches, consider adding more test cases to cover:
- Type mismatches in function arguments
- Invalid type conversions
- Multiple diagnostic messages in a single compilation
Would you like me to help generate additional test cases for these scenarios?
test/expander_test.exs (1)
199-210: LGTM! Clear error messaging for missing return values.The error handling change appropriately uses CompileError for compile-time validation of function return values.
Consider adding a test comment explaining that
dummy/1is intentionally undefined, to make the test's intention clearer:def foo(a :: Term.t()) :: Term.t(), do: func.return(dummy(a)) # dummy/1 is intentionally undefined to test error handlinglib/charms/jit.ex (1)
176-176: Handle Potential Errors When Stopping Diagnostic ServerSimilar to the initialization, consider handling possible errors when stopping the
diagnostic_server. WhileGenServer.stop/1typically returns:ok, handling unexpected cases can enhance error resilience.Modify the code to handle errors gracefully:
case GenServer.stop(diagnostic_server) do :ok -> :ok {:error, reason} -> Logger.warn("Failed to stop diagnostic server: #{inspect(reason)}") endlib/charms/defm/definition.ex (1)
282-296: Consider logging the error before raising the exception for better debugging.Currently, exceptions are raised without logging the error messages. Adding log statements can help with debugging and tracing issues in production environments.
Suggested change:
case MLIR.Pass.Composer.run(op, print: Charms.Debug.step_print?()) do {:ok, op} -> op {:error, msg} -> case Charms.Diagnostic.compile_error_message(diagnostic_server) do {:ok, dm} -> + Logger.error("Compilation error: #{dm}") raise CompileError, dm {:error, _} -> + Logger.error("Compilation error: #{msg}") raise CompileError, file: __ENV__.file, line: __ENV__.line, description: msg end endlib/charms/defm/expander.ex (3)
80-80: Nitpick: Correct grammatical error in documentationIn the docstring of the
expandfunction, consider changing "purpose" to "purposes" for correct grammar.Apply this diff to fix the grammatical error:
- Note that this function will not do any resource management, like destroying MLIR context or module. So this function should be used with care, and mainly for testing or debugging purpose. + Note that this function will not do any resource management, like destroying MLIR context or module. So this function should be used with care, and mainly for testing or debugging purposes.
190-190: Nitpick: Correct grammatical error in error messageIn the
raise_compile_errorcall at line 190, consider improving the error message grammar:- "function call #{name} different return type from its definition" + "function call #{name} has a different return type from its definition"
153-153: Nitpick: Ensure consistent parameter ordering in function definitionsThe functions
infer_by_lookup(line 153) andinfer_by_resolving(line 204) now include theenvparameter. For consistency and readability, consider ordering the parameters uniformly across these functions, particularly the placement ofenvandstate.Also applies to: 204-204
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
- lib/charms/defm/definition.ex (3 hunks)
- lib/charms/defm/expander.ex (21 hunks)
- lib/charms/diagnostic.ex (1 hunks)
- lib/charms/env.ex (1 hunks)
- lib/charms/intrinsic.ex (1 hunks)
- lib/charms/jit.ex (4 hunks)
- lib/charms/pointer.ex (3 hunks)
- lib/charms/prelude.ex (3 hunks)
- lib/charms/simd.ex (2 hunks)
- lib/charms/term.ex (1 hunks)
- test/call_test.exs (1 hunks)
- test/const_test.exs (2 hunks)
- test/defm_test.exs (4 hunks)
- test/expander_test.exs (1 hunks)
🔇 Additional comments (26)
lib/charms/term.ex (1)
8-10: Clarify the relationship to the diagnostic server.The PR title mentions attaching a custom diagnostic server, but it's not immediately clear how this signature change relates to that objective. Could you please explain:
- How this change supports the diagnostic server integration?
- Whether additional changes are needed in this module to fully support the diagnostic server?
Let's check for any diagnostic-related changes in other files:
lib/charms/env.ex (1)
8-8: Verify behavior implementation across modulesThe signature change adds an unused
_paramsparameter, which appears to be part of a broader standardization effort. Let's verify this pattern across other intrinsic modules.✅ Verification successful
All intrinsic modules consistently implement the new parameter pattern
The verification shows that all modules implementing
handle_intrinsicfollow a consistent pattern with the_paramsparameter:
lib/charms/term.ex:handle_intrinsic(:t, _params, [], opts)lib/charms/simd.ex:handle_intrinsic(:t, _params, [type, width], _opts)lib/charms/pointer.ex:handle_intrinsic(:t, _params, [], opts)lib/charms/env.ex:handle_intrinsic(:t, _params, [], opts)lib/charms/prelude.ex:handle_intrinsic(_name, _params, _args, _opts)The change in
env.exis consistent with the standardized parameter pattern used across all intrinsic modules in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent implementation of handle_intrinsic across modules # Expected: All intrinsic modules should follow the same parameter pattern # Find all files using Charms.Intrinsic echo "=== Files using Charms.Intrinsic ===" rg "use Charms.Intrinsic" # Check handle_intrinsic implementations echo -e "\n=== handle_intrinsic implementations ===" ast-grep --pattern 'def handle_intrinsic($$$)'Length of output: 4668
test/const_test.exs (2)
Line range hint
5-17: LGTM! Good practice in isolating compile-time code.The module definition is properly wrapped in a function, which is a good practice for testing compile-time errors. This ensures the code is evaluated at the right time during the test execution.
18-20: LGTM! Appropriate error type and message format.The change from
ArgumentErrortoCompileErroraligns well with the PR's objective of improving diagnostic capabilities. The error message now includes the file location, which enhances debugging experience.test/call_test.exs (1)
Line range hint
10-22: Verify error type consistency.There appears to be an inconsistency between the expected error type and the log message:
- The test expects a
CompileErrorto be raised- However, the log message contains
"(ArgumentError) Unknown intrinsic: AbsentMod.absent_fun/2"This mismatch might indicate incomplete error handling changes or potential issues in the error reporting system.
Let's verify the error handling consistency across the codebase:
✅ Verification successful
Error type change is correct and consistent
The change from
RuntimeErrortoCompileErroris intentional and correct. Here's why:
- The error handling in
lib/charms/defm/definition.exshows thatCompileErroris raised during the compilation phase for invalid definitions and transformations- The log message containing "ArgumentError" is just diagnostic information about the underlying cause (unknown intrinsic), while the actual error type (
CompileError) correctly indicates when this error is caught (during compilation)- This follows the pattern seen in the codebase where
CompileErrorwraps other error types when they occur during the compilation phase🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for similar error handling patterns in other test files # and implementation files to ensure consistency. # Search for ArgumentError mentions in test files echo "Checking ArgumentError usage in tests:" rg "ArgumentError.*Unknown intrinsic" "test/" # Search for CompileError usage with similar patterns echo -e "\nChecking CompileError usage with similar patterns:" rg "CompileError.*Unknown intrinsic" "test/" # Search for error handling in implementation echo -e "\nChecking error handling in implementation:" rg "raise.*Unknown intrinsic" --type elixirLength of output: 538
Script:
#!/bin/bash # Let's check the implementation files to understand how errors are raised for unknown intrinsics # First, let's find files that might handle unknown intrinsics echo "Files containing 'intrinsic' related code:" rg -l "intrinsic" --type elixir # Then, let's examine error handling in the compiler/expander echo -e "\nError handling in compiler/expander:" rg "raise.*Error" --type elixir lib/ # Let's also check for any recent changes in error handling echo -e "\nRecent changes in error handling:" git log -p --grep="error" -i --since="3 months ago"Length of output: 1489
Script:
#!/bin/bash # Let's examine the specific error handling in the expander and intrinsic modules echo "Checking error handling in defm/expander.ex:" rg -B 5 -A 5 "raise.*Error" lib/charms/defm/expander.ex echo -e "\nChecking error handling in intrinsic.ex:" rg -B 5 -A 5 "raise.*Error" lib/charms/intrinsic.ex echo -e "\nChecking how unknown/absent functions are handled:" ast-grep --pattern 'raise $error, $msg' lib/charms/defm/expander.exLength of output: 1311
lib/charms/simd.ex (2)
26-28: LGTM: Parameter signature updated correctlyThe function signature has been updated to include
_paramsparameter, maintaining consistency with the standardization of parameter handling across modules.
Line range hint
10-28: Verify consistent parameter handling across all intrinsic handlersLet's ensure all related intrinsic handlers have been updated to use the new parameter pattern.
✅ Verification successful
All intrinsic handlers follow consistent parameter patterns
The verification shows that all
handle_intrinsicimplementations across the codebase follow a consistent pattern with 4 parameters:
- First parameter: atom for operation type
- Second parameter: params (often unused as
_params)- Third parameter: list of operation-specific arguments
- Fourth parameter: opts map
The changes in
simd.exalign with this pattern, and the call site within the same file (line 19) is also consistent with the new signature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistency in intrinsic handler signatures across the codebase # Search for handle_intrinsic function definitions echo "Checking handle_intrinsic definitions:" rg -A 2 "def handle_intrinsic\(" lib/charms/ # Search for potential call sites that might need updating echo -e "\nChecking handle_intrinsic call sites:" ast-grep --pattern 'handle_intrinsic($$$)'Length of output: 5362
lib/charms/diagnostic.ex (2)
20-25: LGTM!The macro is well-implemented and correctly handles variable hygiene using
unquote.
5-5: Verify Beaver.Diagnostic.Server dependency.Let's ensure that the Beaver dependency is properly declared and the module is available.
✅ Verification successful
Beaver dependency and module usage verified successfully
The verification shows that:
- The Beaver dependency is properly declared in mix.exs with version "~> 0.3.10"
- Beaver.Diagnostic.Server is consistently used across multiple files:
- lib/charms/jit.ex: Server initialization
- lib/charms/diagnostic.ex: Server flush operation (current file)
- lib/charms/defm/definition.ex: Server initialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for Beaver dependency declaration and module usage # Look for mix.exs dependency echo "Checking mix.exs for Beaver dependency..." rg "beaver" mix.exs -A 2 -B 2 # Look for Beaver.Diagnostic.Server usage echo "Checking for other Beaver.Diagnostic.Server usage..." rg "Beaver\.Diagnostic\.Server" --type elixirLength of output: 898
lib/charms/pointer.ex (6)
10-11: LGTM: Clean delegation to maintain backward compatibility.The function elegantly handles the case where size is not specified by defaulting to 1.
52-55: LGTM: Clean implementation of load operation.The function correctly implements the LLVM load operation while following the new parameter pattern.
58-61: LGTM: Clean implementation of store operation.The function correctly implements the LLVM store operation while following the new parameter pattern.
73-75: LGTM: Clean implementation of type operation.The function correctly implements the LLVM pointer type operation while following the new parameter pattern.
Line range hint
64-70: Verify the usage of hardcoded minimum i32 value.The implementation uses -2147483648 as a hardcoded value for
rawConstantIndices. Please verify if this is intentional and document the reasoning.Let's search for any documentation or related code:
#!/bin/bash # Description: Look for documentation or related usage of this constant # Expected: Comments, documentation, or similar patterns explaining this value rg -A 5 "rawConstantIndices.*-2147483648"
Line range hint
14-24: Verify type conversion behavior for large sizes.The implementation now properly handles index types, but we should verify the behavior with large sizes that might exceed i32 range.
Let's check for any existing tests that verify this behavior:
lib/charms/prelude.ex (2)
Line range hint
41-89: Consider adding integer overflow checks for arithmetic operations.The arithmetic operations (add, subtract, multiply) should consider potential integer overflow/underflow, especially since this is part of a compilation process. Consider using MLIR's checked arithmetic operations or adding explicit overflow checks.
Let's check if there are any existing overflow checks in the codebase:
Consider implementing a diagnostic callback for arithmetic overflow detection during compilation, which aligns with the PR's objective of attaching a diagnostic server.
Line range hint
110-110: Validate intrinsic definitions for uniqueness.Consider adding validation to ensure there are no naming conflicts between different types of intrinsics (ENIF functions, binary operators, etc.).
Let's check for potential naming conflicts:
test/defm_test.exs (2)
9-9: LGTM: Error assertions properly updatedThe transition from
ArgumentErrortoCompileErrorwith detailed file paths and line numbers improves error reporting clarity and aligns with the new diagnostic server implementation.Also applies to: 21-21
Line range hint
126-137: LGTM: Comprehensive type checking testThe test effectively verifies the compile-time type checking for remote function calls, with clear error messages that help developers identify type mismatches.
test/expander_test.exs (2)
184-195: LGTM! Improved error handling with helpful suggestions.The change from ArgumentError to CompileError is more appropriate for compile-time issues. The error message now includes a helpful suggestion ("did you mean: cf.br"), which improves the developer experience.
184-210: Verify consistency of error handling across test files.Let's ensure all similar error cases in other test files have been updated to use CompileError consistently.
✅ Verification successful
Error handling is inconsistent across test files
The verification reveals mixed usage of error types:
ArgumentErroris used in:
test/defm_test.exsfor list validationtest/call_test.exsfor unknown intrinsicstest/vec_add_test.exsfor value count validationCompileErroris used consistently for MLIR operation errors, type validation, and function-related compile-time checksLet's verify if these
ArgumentErrorcases should beCompileErrorinstead by checking their context:
Current error type usage is correct - no changes needed
The error types are being used appropriately based on their context:
ArgumentErroris correctly used for runtime validation:
- Input type checking (
list expected)- Vector size validation (
expected 8 values)- Runtime intrinsic resolution (
Unknown intrinsic)CompileErroris correctly used for compile-time issues:
- MLIR operation validation
- Type checking
- Function resolution
The distinction between compile-time and runtime errors is being maintained consistently across the test files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for remaining instances of ArgumentError in test files echo "Checking for remaining ArgumentError usage in tests:" rg "ArgumentError" "test/" -A 2 # Search for assert_raise usage to verify error type consistency echo -e "\nChecking CompileError usage in tests:" rg "assert_raise CompileError" "test/" -A 2Length of output: 2676
Script:
#!/bin/bash # Get more context around ArgumentError usages echo "Context for ArgumentError in defm_test.exs:" rg -B 5 -A 5 "assert_raise ArgumentError.*list expected" test/defm_test.exs echo -e "\nContext for ArgumentError in call_test.exs:" rg -B 5 -A 5 "ArgumentError.*Unknown intrinsic" test/call_test.exs echo -e "\nContext for ArgumentError in vec_add_test.exs:" rg -B 5 -A 5 "assert_raise ArgumentError.*expected 8 values" test/vec_add_test.exsLength of output: 1387
lib/charms/jit.ex (4)
7-7: Update to Struct Definition is CorrectThe addition of
diagnostic_serveranddiagnostic_handler_idto the struct enhances error handling capabilities and ensures that diagnostic information is properly managed throughout the JIT compilation process.
83-98: Enhanced Error Handling Improves RobustnessThe introduction of the
try-rescueblock and the use ofCharms.Diagnostic.compile_error_message/1to extract detailed diagnostic messages significantly improve error reporting during the JIT compilation process. This ensures that users receive more informative compile errors.
99-106: Struct Initialization Includes Diagnostic ComponentsIncluding
diagnostic_serveranddiagnostic_handler_idin the struct initialization ensures that all components related to diagnostics are properly tracked and available for later use, such as in resource cleanup.
169-176: Ensure Proper Cleanup indestroy/1FunctionThe
destroy/1function now appropriately stops thediagnostic_serverand detaches the diagnostic handler before destroying the execution engine and context. This prevents potential resource leaks and ensures that all associated processes are properly terminated.lib/charms/defm/definition.ex (1)
231-233: LGTM!The diagnostic server is correctly started and attached to the MLIR context.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
lib/charms/pointer.ex (2)
Line range hint
14-27: Consider optimizing the type casting logic.The current implementation always creates a 32-bit integer constant and then potentially casts it. For better performance, consider creating the constant with the target type (i64) directly when the size is a compile-time integer.
- size = Arith.constant(value: Attribute.integer(Type.i(32), size)) >>> ~t<i32> + size = Arith.constant(value: Attribute.integer(Type.i(64), size)) >>> ~t<i64> - size = - if MLIR.CAPI.mlirTypeIsAIndex(MLIR.Value.type(size)) |> Beaver.Native.to_term() do - Index.casts(size) >>> Type.i64() - else - size - end
Line range hint
64-71: Document the purpose of the hardcoded index value.The value
-2147483648inrawConstantIndicesappears to be a magic number. Please add a comment explaining its significance or consider making it a named constant if it has special meaning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- lib/charms/pointer.ex (3 hunks)
🔇 Additional comments (5)
lib/charms/pointer.ex (5)
10-11: LGTM! Good use of function delegation.The function maintains backward compatibility while avoiding code duplication by delegating to the more general implementation.
52-56: LGTM! Clean implementation of load operation.The function correctly implements the LLVM load operation while adhering to the new parameter pattern.
58-62: LGTM! Clean implementation of store operation.The function correctly implements the LLVM store operation while adhering to the new parameter pattern.
73-75: LGTM! Clean implementation of type operation.The function correctly returns the LLVM pointer type while adhering to the new parameter pattern.
29-50: Verify runtime type safety for size parameter.While the code handles type conversion for non-i64 sizes, we should ensure that the input size value is always a numeric type at runtime to prevent potential crashes.
✅ Verification successful
Based on the search results, I can now generate a final response. The code shows that there are robust type checks throughout the codebase:
Runtime type safety for size parameter is properly enforced
The codebase has multiple layers of type safety for MLIR values:
- The
handle_intrinsicfunction explicitly pattern matches on%MLIR.Type{}and%MLIR.Value{}- The expander module (
lib/charms/defm/expander.ex) validates all arguments are proper MLIR values before processing- Type conversion is properly handled with explicit checks against
i64type- Invalid arguments are caught and handled by creating poison values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all call sites of the allocate function to verify type safety ast-grep --pattern 'handle_intrinsic(:allocate, $_, $_)'Length of output: 58
Script:
#!/bin/bash # Let's try a different approach to find allocate usage patterns # First, search for direct references to allocate rg -l "allocate" --type elixir # Then look for MLIR value handling patterns rg "MLIR\.Value" --type elixir -A 2 -B 2Length of output: 4103
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
CompileErrorinstead ofArgumentError, ensuring more accurate error reporting during compilation.Documentation
Tests