Macro-based intrinsic #48
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on the restructuring of intrinsic function definitions and enhancements to error handling. Key changes include the removal of Changes
Possibly related PRs
Poem
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: 12
🧹 Outside diff range and nitpick comments (14)
lib/charms/env.ex (1)
Line range hint
1-11: Good architectural direction with defintrinsic macro adoptionThis refactoring aligns well with the broader effort to standardize intrinsic function definitions across the codebase. The
defintrinsicmacro approach provides several benefits:
- Consistent pattern for defining intrinsics
- Better type safety through explicit return type declarations
- Cleaner context handling through pattern matching
Consider documenting this pattern in the project's contribution guidelines to ensure consistency in future intrinsic implementations.
bench/vec_add_int_list.ex (1)
8-9: Consider addressing the TODO commentSince this PR deals with type-related improvements, it might be a good opportunity to address the TODO comment about pointer type inference.
Would you like me to help investigate if pointer type inference is now possible and create a follow-up issue for this enhancement?
test/defm_test.exs (2)
176-188: LGTM: Good test coverage for type checkingThe test case effectively verifies the compiler's type checking for negation operations. Consider adding a brief comment explaining why floating-point negation is unsupported to improve documentation.
Add a comment before the test:
+ # Verifies that the compiler correctly prevents negation of floating-point numbers, + # which is an unsupported operation in the current implementation. test "negate" do
190-205: Consider adding more test cases for type_ofWhile the current test case demonstrates basic functionality, consider adding tests for:
- Error handling when
type_ofis used with invalid types- Edge cases with different numeric types
- Interaction with other operators
Example additional test cases:
test "type_of with invalid types" do assert_raise CompileError, ~r/invalid type/, fn -> defmodule TypeOfInvalid do use Charms defm foo(env) do type_of(:atom) # Should fail at compile time end end end end test "type_of with different numeric types" do # Test with i64, f64, etc. endlib/charms/simd.ex (2)
21-23: Use a specific exception type when raising errorsCurrently, the code raises a generic exception with a string message when the
element_typeis null. It's better practice to raise a specific exception likeArgumentErrorfor clearer error handling.Apply this diff to use
ArgumentError:if MLIR.is_null(element_type) do - raise "element type is null" + raise ArgumentError, "element type is null" end
27-28: Ensure consistent use ofEnum.countIn line 27, you use
Enum.count(literal_values), while in line 28, you uselength(literal_values). For consistency and clarity, it's preferable to use one method throughout. Given thatliteral_valuesis an enumerable,Enum.count/1is generally preferred.Apply this diff to use
Enum.countconsistently:if Enum.count(literal_values) != width do - raise ArgumentError, "expected #{width} values, got #{length(literal_values)}" + raise ArgumentError, "expected #{width} values, got #{Enum.count(literal_values)}" endlib/charms/prelude.ex (2)
Line range hint
10-19: Logical inconsistency in pattern matching withinwrap_argThe function
wrap_arg/2is defined with a guard clausewhen is_integer(i), ensuring thatiis always an integer. However, inside thecaseexpression, you pattern match on%MLIR.Value{}as one of the cases fori. Sinceiis guaranteed to be an integer, the pattern match for%MLIR.Value{}will never succeed, rendering that clause ineffective.Consider removing the guard clause from the function head to allow
ito match both integers and%MLIR.Value{}structs. Here is a suggested correction:-defp wrap_arg({i, t}, %Opts{ctx: ctx, block: block}) when is_integer(i) do +defp wrap_arg({i, t}, %Opts{ctx: ctx, block: block}) do mlir ctx: ctx, block: block do case i do %MLIR.Value{} -> i i when is_integer(i) -> if MLIR.CAPI.mlirTypeIsAInteger(t) |> Beaver.Native.to_term() do Arith.constant(value: Attribute.integer(t, i)) >>> t else raise ArgumentError, "Not an integer type, #{to_string(t)}" end end end endThis change removes the guard clause, allowing
ito be either an integer or an%MLIR.Value{}, making the pattern matching meaningful.
44-44: Ensure proper management of MLIR contextCreating (
MLIR.Context.create()) and destroying (MLIR.Context.destroy(ctx)) the MLIR context within the module is crucial for resource management. However, repeatedly creating and destroying contexts can be inefficient.Consider reusing an existing context or managing the context outside of this module if possible. If the context is shared across multiple modules or operations, passing it as a parameter or using a singleton pattern might improve performance.
Also applies to: 68-68
lib/charms/kernel.ex (1)
69-71: Useunlessinstead ofif notfor clarityFor improved readability, consider replacing
if notwithunlesswhen checking conditions.Apply this diff:
- if not MLIR.equal?(MLIR.Value.type(left), MLIR.Value.type(right)) do + unless MLIR.equal?(MLIR.Value.type(left), MLIR.Value.type(right)) do raise "args of binary op must be same type" endlib/charms/intrinsic.ex (1)
85-88: Handle potential mismatches in pattern matchingcallindefintrinsic/3.Directly pattern matching
{name, _meta, args} = callassumes thatcallis a tuple with at least three elements. To prevent potential errors whencalldoes not match this structure, consider usingMacro.decompose_call/1for safer decomposition.Suggested change:
defmacro defintrinsic(call, opts, do: body) do {name, args} = Macro.decompose_call(call) # Rest of the code...lib/charms/defm/expander.ex (4)
708-727: Preserve stack trace when re-raising exceptions during intrinsic expansionIn the
expand/3function, exceptions caught during intrinsic expansion are re-raised usingraise_compile_error, which can discard the original stack trace:rescue e -> raise_compile_error( env, "Failed to expand kernel intrinsic #{fun}: #{Exception.message(e)}" )Consider using
reraiseto maintain the original stack trace, aiding in debugging.Apply this diff:
rescue e -> - raise_compile_error( - env, - "Failed to expand kernel intrinsic #{fun}: #{Exception.message(e)}" - ) + reraise "Failed to expand kernel intrinsic #{fun}: #{Exception.message(e)}", __STACKTRACE__
1126-1129: Useis_struct/2instead ofmatch?/2for type checkingIn the
expand_macrofunction handling the:whilemacro:unless match?(%MLIR.Value{}, condition) do raise_compile_error(env, "Expected a value, got: #{inspect(condition)}") endUsing
is_struct/2provides a clearer intent when checking ifconditionis anMLIR.Valuestruct.Apply this diff:
- unless match?(%MLIR.Value{}, condition) do + unless is_struct(condition, MLIR.Value) do raise_compile_error(env, "Expected a value, got: #{inspect(condition)}") end
1299-1319: Refactor duplicate code for intrinsic function handlingThe logic for handling intrinsic functions in
expand_remoteduplicates checks performed elsewhere:cond do Code.ensure_loaded?(module) and function_exported?(module, :__intrinsics__, 2) and module.__intrinsics__(fun, length(args)) -> expand_intrinsics(loc, module, fun, args, state, env) # ... endConsider extracting this repeated pattern into a helper function, such as
module_supports_intrinsic?/3, to improve code maintainability.Example:
defp module_supports_intrinsic?(module, fun, arity) do Code.ensure_loaded?(module) and function_exported?(module, :__intrinsics__, 2) and module.__intrinsics__(fun, arity) endRefactor the condition:
- Code.ensure_loaded?(module) and function_exported?(module, :__intrinsics__, 2) and - module.__intrinsics__(fun, length(args)) -> + module_supports_intrinsic?(module, fun, length(args)) ->
1328-1338: Preserve stack trace when handling exceptions inexpand_localIn the
expand_localfunction, exceptions are caught and a new error is raised, potentially losing the original stack trace:rescue e -> raise_compile_error( env, "Failed to expand local function #{fun}/#{length(args)}: #{Exception.message(e)}" ) endUse
reraiseto maintain the original stack trace, aiding in debugging.Apply this diff:
rescue e -> - raise_compile_error( - env, - "Failed to expand local function #{fun}/#{length(args)}: #{Exception.message(e)}" - ) + reraise "Failed to expand local function #{fun}/#{length(args)}: #{Exception.message(e)}", __STACKTRACE__
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
.formatter.exs(0 hunks)bench/vec_add_int_list.ex(1 hunks)lib/charms.ex(2 hunks)lib/charms/defm/expander.ex(6 hunks)lib/charms/env.ex(1 hunks)lib/charms/intrinsic.ex(2 hunks)lib/charms/kernel.ex(1 hunks)lib/charms/pointer.ex(2 hunks)lib/charms/prelude.ex(2 hunks)lib/charms/simd.ex(1 hunks)lib/charms/term.ex(1 hunks)test/const_test.exs(1 hunks)test/defm_test.exs(3 hunks)test/expander_test.exs(2 hunks)test/vec_add_test.exs(1 hunks)
💤 Files with no reviewable changes (1)
- .formatter.exs
🔇 Additional comments (30)
lib/charms/env.ex (2)
6-6: LGTM: Clean alias declaration
The alias for Charms.Intrinsic.Opts improves code readability and follows Elixir conventions.
8-10: Verify the intrinsic function behavior
The implementation looks correct, with proper pattern matching on the Opts struct and context passing. However, we should verify that this change maintains compatibility with existing callers.
✅ Verification successful
Let me gather more context about the usage in lib/charms/defm/definition.ex to understand how it's being used.
Based on the context, I can now generate the final response:
The intrinsic function change is safe and compatible
The usage of Charms.Env.t() is only found in the type specification within lib/charms/defm/definition.ex where it's used to annotate the env parameter type in function signatures. This is a type-level usage that's compatible with the implementation change in lib/charms/env.ex. The intrinsic function maintains the same type signature and behavior by properly handling the context through the Opts struct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for existing usages of Charms.Env.t() to verify compatibility
# Look for both direct calls and type specifications
# Search for function calls
rg -t elixir "Charms\.Env\.t\(\)"
# Search for type specs using Charms.Env.t()
ast-grep --pattern '@type $_ :: $$$Charms.Env.t()$$$'
Length of output: 164
Script:
#!/bin/bash
# Get more context around the usage in definition.ex
rg -A 5 -B 5 "Charms\.Env\.t\(\)" lib/charms/defm/definition.ex
# Look for the module definition and any related type specs
ast-grep --pattern 'defmodule Charms.Defm.Definition do
$$$
'
Length of output: 21203
lib/charms/term.ex (3)
3-3: LGTM: Documentation accurately reflects module purpose
The updated documentation correctly specifies this as an Erlang term type intrinsic module.
6-6: LGTM: Appropriate module setup and alias usage
The module setup with necessary aliases is clean and follows conventions.
11-12: LGTM: Clean implementation of the new intrinsic pattern
The implementation correctly uses the new macro-based approach and properly handles the context.
Let's verify this new pattern is consistently applied across other intrinsic modules:
✅ Verification successful
Let me gather more information to verify the pattern consistency across the codebase.
Consistent pattern usage confirmed across intrinsic modules
The implementation in term.ex follows the established pattern that is consistently used across all charm modules. The search results show that:
- No old-style
handle_intrinsicfunctions remain - All
defintrinsicusages follow the same pattern with proper context handling - The implementation matches other similar type-related intrinsics (e.g. in
env.ex)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in intrinsic definitions across modules
# Look for any remaining handle_intrinsic functions that should be migrated
# Search for old-style handle_intrinsic definitions
echo "Checking for remaining handle_intrinsic definitions:"
rg "def handle_intrinsic" lib/charms/
# Search for new-style defintrinsic usage
echo -e "\nVerifying consistent defintrinsic usage:"
ast-grep --pattern 'defintrinsic $name($$$), %Opts{ctx: ctx} do
$$$
end'
Length of output: 328
Script:
#!/bin/bash
# Search for all defintrinsic usages to verify pattern consistency
echo "Checking all defintrinsic usages:"
rg "defintrinsic" lib/charms/ -A 2
# Also check for all files in charms directory to ensure we're not missing anything
echo -e "\nListing all charm modules:"
fd . lib/charms/ -e ex
Length of output: 4444
test/vec_add_test.exs (2)
11-11: LGTM! Appropriate exception type for compile-time validation.
The change from ArgumentError to CompileError is more accurate since the vector size mismatch is detected during compilation.
17-17: Verify test coverage for all error scenarios.
The test effectively verifies the size mismatch error case. However, let's ensure we have coverage for other potential error scenarios.
✅ Verification successful
Let me gather more information about the test coverage.
Let me try one more search to specifically look for SIMD-related test cases.
Test coverage for SIMD operations appears sufficient
Based on the codebase analysis, the current test coverage adequately handles the critical error scenarios for SIMD operations:
- Size mismatch error is properly tested (attempting to initialize with 6 values when 8 are expected)
- The test file includes positive test cases for vector addition
- The SIMD module is used consistently across the codebase with proper type checking
The CompileError being raised for size mismatch is the primary validation needed for SIMD vector initialization, as other type-related errors would be caught by the Elixir compiler's type system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other size-related test cases in the codebase
# Look for similar test patterns
rg -A 5 "assert_raise.*CompileError.*expected.*values" --type exs
# Check for other SIMD initialization test cases
ast-grep --pattern 'SIMD.new(SIMD.t(i32(), $_), $_)'
Length of output: 325
Script:
#!/bin/bash
# Look for test files and their content
rg -l "test" --type elixir
rg "test.*SIMD" --type elixir -A 5
rg "assert_raise" --type elixir -A 5
# Look for SIMD-related error handling
rg "raise.*SIMD" --type elixir
rg "CompileError.*SIMD" --type elixir
Length of output: 4940
Script:
#!/bin/bash
# Look for SIMD-related test cases
cat test/vec_add_test.exs
# Look for SIMD-related error handling in source
rg "SIMD" --type elixir -B 2 -A 5
Length of output: 2236
test/const_test.exs (1)
19-19: LGTM! The regex pattern improves test flexibility.
The change from string literal to regex pattern allows for more robust error message matching.
Let's verify the consistency of this pattern across other test files:
✅ Verification successful
Regex pattern usage for error messages is consistent across test files
The verification shows that:
- No test files use string literals for error message assertions with file:line format
- The regex pattern style is consistently used in other test files (e.g., vec_add_test.exs)
- The change aligns with the existing codebase conventions for error message testing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar error message assertions in test files
# Expected: Find similar patterns of error message assertions using regex
# Search for assert_raise with string literals that might need updating
rg -t exs 'assert_raise.*".*:.*:.*"' test/
# Search for assert_raise with regex patterns to confirm consistency
rg -t exs 'assert_raise.*~r"' test/
Length of output: 138
Script:
#!/bin/bash
# Search for assert_raise with string literals that might need updating
rg 'assert_raise.*".*:.*:.*"' --type-add 'elixir:*.{ex,exs}' --type elixir test/
# Search for assert_raise with regex patterns to confirm consistency
rg 'assert_raise.*~r"' --type-add 'elixir:*.{ex,exs}' --type elixir test/
Length of output: 247
bench/vec_add_int_list.ex (2)
10-10: SIMD initialization change looks good!
The new initialization approach using SIMD.t(i32(), 8) and a list of values is more idiomatic and type-safe compared to the previous direct argument approach.
Let's verify that this change doesn't affect the behavior of dependent functions:
✅ Verification successful
SIMD initialization change is correct and well-tested
The verification confirms:
- The new SIMD initialization syntax is consistent with the test case that validates vector size (8 elements required)
- The change is properly type-annotated with
SIMD.t(i32(), 8) - The
load_listfunction is used correctly in bothaddanddummy_load_no_makefunctions - The test suite includes validation for vector addition and incorrect initialization scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other SIMD vector initializations that might need similar updates
# and verify test coverage for the changed initialization pattern
# Look for similar SIMD initialization patterns
rg "SIMD\.new.*\(" --type elixir
# Look for related test cases
rg "test.*load_list" --type elixir
Length of output: 240
Script:
#!/bin/bash
# Let's check the test file content to understand the test coverage
cat test/vec_add_test.exs
# Also check the implementation file to understand the context
cat bench/vec_add_int_list.ex
# Look for any other files that might be using SIMD operations
fd -e ex -e exs | xargs rg "SIMD\." --type elixir
Length of output: 2450
Line range hint 12-19: Consider pointer cleanup in the reduce loop
The v_ptr is allocated in each iteration of the reduce loop but there's no explicit cleanup. While this might be handled by the framework, it would be good to verify the memory management strategy.
Let's check how pointer cleanup is handled in similar patterns:
lib/charms.ex (2)
11-14: Documentation improvements look good!
The added clarification about intrinsic functions vs defm helps developers make informed decisions about which approach to use. The documentation clearly explains that intrinsics can return MLIR types and are always inlined, while defm is more suitable for simpler functions.
39-39: Verify the impact of importing Charms.Prelude
The addition of import Charms.Prelude will affect all modules that use Charms. Let's verify:
- The contents of
Charms.Preludeto understand what's being imported - Potential naming conflicts in existing modules
✅ Verification successful
Let me gather more information to understand potential naming conflicts by checking what functions are being imported and how they're used.
Import of Charms.Prelude appears safe to use
The import is well-scoped and safe because:
Charms.Preludeonly defines a small set of intrinsic functions (result_at,type_of) and dynamically generated ENIF functions- These functions are specialized with clear purposes (MLIR operations, pointer operations)
- Existing usage shows these functions are already being used throughout the codebase without conflicts
- The functions are domain-specific enough that naming conflicts are unlikely
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the contents of Charms.Prelude and its usage
# Check what's defined in Charms.Prelude
ast-grep --pattern 'defmodule Charms.Prelude do
$$$
end'
# Find existing usages of Charms to check for potential conflicts
rg -l "use Charms"
Length of output: 4437
Script:
#!/bin/bash
# Check what functions are defined in Charms.Prelude
ast-grep --pattern 'defintrinsic $name($$$), $$$'
# Check for any direct usage of functions from Charms.Prelude in existing files
rg "Charms\.Prelude\." -A 2
# Check for potential conflicts by looking for similar function names
rg "result_at|type_of" -A 2
Length of output: 1675
test/expander_test.exs (2)
169-169: LGTM! Good use of regex pattern matching
The change from a string literal to a regex pattern (~r"example.exs: Unknown MLIR operation to create: cf.ar, did you mean: cf.br") makes the test more resilient to minor variations in error messages while still ensuring the core error case is properly tested.
184-184: LGTM! Consistent error message pattern matching
Similar to the previous change, using a regex pattern (~r"example.exs: Function call @Elixir.InvalidLocalCall.dummy does not return a value") improves the test's robustness while maintaining its effectiveness in catching the no-return error case.
test/defm_test.exs (1)
52-52: LGTM: Improved error message assertions
The switch to regex patterns for error message assertions is a good improvement that makes the tests more maintainable and less brittle to minor message changes.
Also applies to: 66-66
lib/charms/simd.ex (2)
8-8: Addition of alias for Charms.Intrinsic.Opts
The alias Charms.Intrinsic.Opts improves code readability by allowing shorter references to Opts.
37-43: Definition of defintrinsic t/2 is correct
The defintrinsic t/2 function correctly returns the vector type given type and width. The implementation aligns with the intrinsic handling approach.
lib/charms/prelude.ex (3)
6-6: LGTM: Addition of the Charms.Intrinsic.Opts alias
The addition of the alias enhances code readability and simplifies references to Opts throughout the module.
40-41: LGTM: Addition of type_of intrinsic
The type_of intrinsic provides a convenient way to obtain the type of an MLIR value, enhancing the module's functionality.
70-73: LGTM: Updating intrinsics/0 function to include :result_at
Including :result_at in the list of intrinsics ensures it is recognized and available for use, aligning with the newly defined intrinsic functions.
lib/charms/pointer.ex (4)
74-79: Verify the use of rawConstantIndices value
In the element_ptr function, rawConstantIndices is set to ~a{array<i32: -2147483648>}. This value represents the minimum 32-bit integer and might not be intended.
Please confirm that -2147483648 is the correct value for rawConstantIndices. If it's a placeholder or an unusual constant, it might cause unexpected behavior.
If a default or zero value is intended, consider adjusting it:
52-55: load intrinsic function implementation is correct
The load function correctly loads a value of the specified type from the given pointer.
61-64: store intrinsic function implementation is correct
The store function properly stores the given value at the specified pointer location.
86-88: t/0 intrinsic function correctly returns the pointer type
The t/0 function appropriately returns the pointer type using Beaver.Deferred.create.
lib/charms/intrinsic.ex (5)
2-7: Definition of Opts struct is appropriate.
The Opts struct is properly defined with the necessary fields to encapsulate options for intrinsic functions.
24-24: Module attribute @intrinsic registered correctly.
Registering the @intrinsic attribute with accumulate: true ensures that all intrinsic definitions are collected appropriately.
28-30: defintrinsic/2 macro correctly delegates to defintrinsic/3.
The defintrinsic/2 macro provides a convenient way to define intrinsics without explicitly passing options by defaulting to an empty %Charms.Intrinsic.Opts{} struct.
89-97: Verify arity calculation in placeholder function.
The arity calculation using length([unquote_splicing(placeholder_args)]) may not accurately reflect the actual number of arguments, especially when dealing with unquote_splicing. Consider alternative methods to ensure the arity is correctly determined.
Please double-check the arity calculation logic to confirm it handles all cases correctly, including when placeholder_args includes spliced arguments.
111-123: Ensure uniqueness in collected intrinsics in __before_compile__/1 macro.
The macro correctly collects intrinsic definitions and removes duplicates using Enum.uniq(). This ensures that the __intrinsics__/2 function handles each intrinsic function accurately.
lib/charms/defm/expander.ex (1)
367-367: Ensure proper handling of empty argument lists in expand_body
In the expand_body function, the line:
Range.new(0, length(args) - 1, 1)
|> Enum.map(&MLIR.Block.get_arg!(Beaver.Env.block(), &1))When length(args) is zero, length(args) - 1 results in -1, which creates an empty range. This means that arg_values will be an empty list when there are no arguments. Ensure that this scenario is correctly handled and does not lead to unintended behavior.
Run the following script to check if any calls to expand_body occur with an empty args list:
Please verify that expand_body behaves correctly when args is empty.
✅ Verification successful
Empty argument lists are safely handled in expand_body
The implementation is correct and safe for empty argument lists. Here's why:
- When
argsis empty,Range.new(0, -1, 1)creates an empty range in Elixir - The subsequent
Enum.mapon an empty range produces an empty list - The
Enum.zipoperation safely handles empty lists, resulting in an empty list of pairs - The
Enum.reduceon an empty list returns the initial state unchanged
This is the expected and correct behavior as it means no argument bindings are created when there are no arguments, which is exactly what we want.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for calls to expand_body with an empty args list.
# Test: Find occurrences where expand_body is called with an empty list of arguments.
rg 'expand_body\(\s*\[\s*\],' lib/
Length of output: 36
Script:
#!/bin/bash
# Let's first understand the context better by examining the expand_body function implementation
ast-grep --pattern 'def expand_body($$$)'
# Also search for any calls to expand_body with any argument pattern
rg 'expand_body\(' lib/ -A 2
# Let's also check if there are any tests related to expand_body
rg 'expand_body' test/ -A 2
Length of output: 593
Script:
#!/bin/bash
# Let's examine the full context around the Range.new usage in expand_body
rg -B 10 -A 10 'Range\.new\(0, length\(args\)' lib/
# Also check how args is used in expand_body
ast-grep --pattern 'expand_body($$$) do
$$$
$args
$$$
end'
# Look for any error handling or guards related to args
rg 'when.*args|args.*when' lib/charms/defm/expander.ex
Length of output: 1755
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (17)
lib/charms/simd.ex (2)
Line range hint
1-8: Consider enhancing module documentationThe current documentation could be expanded to include:
- Common use cases for SIMD operations
- Examples of vector operations
- Performance implications
41-44: Enhance function documentationWhile the docstring explains the purpose, it could be improved by documenting:
- Parameter
type: The element type of the vector- Parameter
width: The size of the vector- Return value: The resulting vector type
@doc """ Return the vector type of the given `type` and `width` + + ## Parameters + + - type: The element type of the vector + - width: The size of the vector + + ## Returns + + A vector type of the specified element type and width """lib/charms/pointer.ex (2)
Line range hint
23-44: Consider adding explicit error handling for invalid size typesWhile the implementation correctly handles integers and MLIR i64 values, it would be beneficial to add explicit error handling for invalid size types.
Consider adding an else clause to handle invalid types:
%MLIR.Value{} -> if MLIR.equal?(MLIR.Value.type(size_v), Type.i64(ctx: ctx)) do size else quote do value arith.extsi(unquote(size)) :: i64() end end + _ -> + raise ArgumentError, "size must be an integer or MLIR i64 value" end
81-86: Enhance function documentationThe current documentation "Return the pointer type" could be more descriptive. Consider adding details about:
- The specific type being created
- When and how this function should be used
- Any constraints or requirements
lib/charms/kernel.ex (7)
1-10: Enhance module documentation with supported operationsThe module documentation could be more comprehensive by including:
- List of supported binary operations
- List of supported unary operations
- Examples of usage
- Type constraints for operands
Apply this diff:
@moduledoc """ Intrinsic module to define functions in `Kernel`. + +Supports the following operations: +- Binary operations: #{inspect(@binary_ops)} +- Binary macro operations: #{inspect(@binary_macro_ops)} +- Unary operations: #{inspect(@unary_ops)} + +All operations work with integer types only. Each operation ensures type +compatibility between operands. + +## Examples + + iex> value = const 42 :: i32() + iex> result = value + 1 """
12-22: Add documentation for helper functionThe
constant_of_same_type/3function would benefit from documentation explaining its purpose, parameters, and return value.Add this documentation before the function:
+@doc """ +Creates a constant value of the same type as a given MLIR value. + +## Parameters + * `i` - Integer value to convert + * `v` - MLIR value to match type with + * `opts` - Options struct containing context, block, and location + +## Returns + MLIR constant value of the same type as `v` + +## Raises + * `ArgumentError` - If the target type is not an integer type +""" defp constant_of_same_type(i, v, %Opts{ctx: ctx, block: block, loc: loc}) do
24-30: Refactor predicate mapping to use a mapConsider using a module attribute map for better maintainability and performance.
Apply this diff:
@compare_ops [:!=, :==, :>, :>=, :<, :<=] -defp i_predicate(:!=), do: :ne -defp i_predicate(:==), do: :eq -defp i_predicate(:>), do: :sgt -defp i_predicate(:>=), do: :sge -defp i_predicate(:<), do: :slt -defp i_predicate(:<=), do: :sle +@predicates %{ + :!= => :ne, + :== => :eq, + :> => :sgt, + :>= => :sge, + :< => :slt, + :<= => :sle +} +defp i_predicate(op), do: @predicates[op]
32-58: Add documentation and type specs for create_binaryThe function would benefit from documentation explaining its purpose and type specifications.
Add this documentation and type spec:
+@doc """ +Creates a binary operation in MLIR based on the operator type. + +## Parameters + * `op` - The operator atom (e.g., :+, :-, :*, etc.) + * `operands` - List of two MLIR values + * `type` - The MLIR type for the operation + * `ctx` - MLIR context + * `block` - MLIR block + * `loc` - MLIR location + +## Returns + MLIR value representing the binary operation result + +## Raises + * `ArgumentError` - If the operator is not supported +""" +@spec create_binary(atom(), [MLIR.Value.t()], MLIR.Type.t(), MLIR.Context.t(), MLIR.Block.t(), MLIR.Location.t()) :: + MLIR.Value.t() defp create_binary(op, operands, type, ctx, block, loc) do
60-86: Consider additional type validation and performance optimizationThe binary operations implementation could be enhanced in several ways:
- Add validation for integer overflow
- Consider caching the type check results
- Add support for constant folding when both operands are constants
Would you like me to provide a detailed implementation for these improvements?
88-99: Optimize logical not implementationConsider using
arith.constantandarith.cmpifor a more idiomatic MLIR implementation of logical not.Apply this diff:
defintrinsic !_value, %Opts{args: [v]} do t = MLIR.Value.type(v) unless MLIR.CAPI.mlirTypeIsAInteger(t) |> Beaver.Native.to_term() do raise ArgumentError, "Not an integer type to negate, unsupported type: #{to_string(t)}" end quote bind_quoted: [v: v, t: t] do - one = const 1 :: t - value arith.xori(v, one) :: t + zero = const 0 :: t + value arith.cmpi(v, zero, predicate: Arith.cmp_i_predicate(:eq)) :: Type.i1() end end
101-109: Add meaningful documentation for public functionsReplace
@doc falsewith meaningful documentation explaining the purpose of these functions.Apply this diff:
-@doc false +@doc """ +Returns the list of supported binary operations. +These operations can be used directly in arithmetic expressions. +""" def intrinsics() do @binary_ops end -@doc false +@doc """ +Returns the list of supported macro operations and unary operations. +These operations require special handling in the macro system. +""" def macro_intrinsics() do @binary_macro_ops ++ @unary_ops endtest/defm_test.exs (1)
190-205: Consider adding error cases for type_ofThe test demonstrates good usage of the
type_offunction with pointer allocation and type inference. However, consider adding test cases for error scenarios:
- Invalid type inference cases
- Type mismatches in pointer operations
Example test case structure:
test "type_of with invalid types" do assert_raise CompileError, ~r/expected valid type/, fn -> defmodule TypeOfInvalid do use Charms defm foo() do type_of(:invalid) end end end endlib/charms/defm/expander.ex (5)
367-367: Consider usingEnum.with_index/1for a more idiomatic approach.Instead of creating a range manually, consider using
Enum.with_index/1which is more idiomatic in Elixir.-Range.new(0, length(args) - 1, 1) -|> Enum.map(&MLIR.Block.get_arg!(Beaver.Env.block(), &1)) +Enum.with_index(args) +|> Enum.map(fn {_, i} -> MLIR.Block.get_arg!(Beaver.Env.block(), i) end)
471-517: Add documentation for return type handling and improve error messages.The intrinsic function expansion logic handles different return types but could benefit from:
- Documentation explaining the expected return types and their handling
- More specific error messages for different failure cases
Add documentation above the function:
@doc """ Expands intrinsic functions with the following return type handling: - MLIR.Value: Used directly - MLIR.Type: Used for type information - MLIR.Operation: Used for operation creation - Function: Called with the current state - AST block: Expanded and last value used - Single AST: Expanded directly """Improve error messages:
- "Unexpected return type from intrinsic #{module}.#{intrinsic_impl}: #{inspect(other)}" + "Invalid return from intrinsic #{module}.#{intrinsic_impl}. Expected MLIR.Value, MLIR.Type, MLIR.Operation, Function, or AST but got: #{inspect(other)}"
705-725: Enhance error message for kernel intrinsic failures.The error message could be more specific about what caused the failure.
- "Failed to expand kernel intrinsic #{fun}: #{Exception.message(e)}" + "Failed to expand kernel intrinsic #{fun}/#{length(args)}: #{Exception.message(e)}. " <> + "Ensure the intrinsic is properly defined and the arguments are valid."
1297-1337: Improve error messages for remote function expansion failures.The error messages could provide more context about why the function wasn't found and potential solutions.
- raise_compile_error(env, "function #{module}.#{fun}/#{length(args)} not found") + raise_compile_error( + env, + "function #{module}.#{fun}/#{length(args)} not found. " <> + "Ensure the module is loaded and the function is exported with the correct arity." + )- "Failed to expand local function #{fun}/#{length(args)}: #{Exception.message(e)}" + "Failed to expand local function #{fun}/#{length(args)}: #{Exception.message(e)}. " <> + "Check if the function is defined and accessible in the current scope."
1114-1128: Enhance error message for invalid while loop condition.The error message could provide more context about what constitutes a valid condition.
- raise_compile_error(env, "Expected a value, got: #{inspect(condition)}") + raise_compile_error( + env, + "Invalid while loop condition: expected an MLIR.Value representing a boolean condition, " <> + "but got: #{inspect(condition)}. Ensure the condition evaluates to a value that can be used for control flow." + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
lib/charms/defm/expander.ex(5 hunks)lib/charms/kernel.ex(1 hunks)lib/charms/pointer.ex(2 hunks)lib/charms/prelude.ex(2 hunks)lib/charms/simd.ex(1 hunks)test/defm_test.exs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/charms/prelude.ex
🔇 Additional comments (10)
lib/charms/simd.ex (2)
21-33: Well-implemented error handling!
The code now includes comprehensive validation:
- Checks for null element type
- Validates width/value count match
- Ensures positive width
35-37: Verify MLIR type operations
The MLIR operations for creating dense elements and constants should be verified for correctness.
✅ Verification successful
MLIR type operations are consistent with codebase patterns
The MLIR operations for creating dense elements and constants in the code are verified to be correct based on the following evidence:
- The pattern of using
Attribute.dense_elementsfollowed byArith.constantis consistent with the codebase's approach to handling MLIR operations - Type checking is properly enforced across the codebase, as seen in multiple files where MLIR type validations are performed using
MLIR.CAPI.mlirTypeIsAIntegerandMLIR.CAPI.mlirTypeIsAFloat - The usage of
Attributemodule for creating integer values and constants follows the same pattern across different parts of the codebase (lib/charms/kernel.ex, lib/charms/defm/expander.ex) - The operation's type inference using the
>>>operator is consistently applied throughout the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar MLIR dense elements and constant operations patterns
ast-grep --pattern 'Attribute.dense_elements($_, $type, ctx: $_)'
Length of output: 67
Script:
#!/bin/bash
# Search for MLIR dense elements and constant operations with more flexible pattern
rg -U "Attribute\.dense_elements.*ctx:" -A 2
# Search for MLIR constant operations
rg -U "Arith\.constant.*type" -A 2
# Search for Attribute module usage
rg "defmodule.*Attribute" -A 5
Length of output: 1110
Script:
#!/bin/bash
# Search for Attribute module definition and functions
fd -e ex -x grep -l "Attribute\." {} \;
# Get content of files that use Attribute module
fd -e ex -x grep -A 10 "Attribute\." {} \;
# Search for MLIR type operations and verifications
rg "MLIR\.CAPI\.mlirType" -A 2
Length of output: 7824
lib/charms/pointer.ex (5)
7-9: LGTM: Well-organized module aliases
The added aliases are properly structured and necessary for the module's functionality.
11-17: LGTM: Clean implementation of single-element allocation
The function is well-documented and provides a convenient wrapper for allocating a single element by delegating to allocate/2.
47-54: LGTM: Clean implementation of load operation
The function is well-documented and correctly implements the pointer load operation.
56-63: LGTM: Clean implementation of store operation
The function is well-documented and correctly implements the pointer store operation.
65-78: Document the significance of the hardcoded constant index
The implementation uses a hardcoded constant index -2147483648 in rawConstantIndices. Please document why this specific value is used and what it represents.
test/defm_test.exs (3)
52-52: LGTM! Improved test maintainability
Good improvement using dynamic line numbers in error patterns. This makes the tests more maintainable as they won't break when line numbers change.
Also applies to: 66-66
68-74: LGTM! Improved test clarity
The module rename from InvalidRet to InvalidArgType better reflects the test's purpose. The type annotation changes correctly test the compile-time type checking for invalid argument types.
176-188: LGTM! Well-structured type checking test
The test properly validates the compile-time type checking for the negation operator with a clear error message.
Summary by CodeRabbit
Release Notes
New Features
Charms.Kernelmodule with intrinsic functions for kernel operations.Charms.PreludeandCharms.SIMDfor improved handling of operations.Documentation
Charmsmodule to clarify the differences between function types and improve overall clarity.Bug Fixes
Charms.Defm.Expandermodule.Tests
DefmTestmodule and modified existing tests for better error handling.