Use __IR__ in defintrinsic
#49
Conversation
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: 1
🧹 Outside diff range and nitpick comments (9)
lib/charms/env.ex (1)
8-9: LGTM! Good refactoring of context handling.The change improves code organization by moving the pattern matching from the function signature to the function body. This is consistent with similar changes across other modules and makes the function signature cleaner while maintaining the same functionality.
This pattern of using
__IR__for context access provides better separation of concerns and makes the module's interface simpler and more maintainable.lib/charms/term.ex (1)
11-12: LGTM! Clean refactoring of context handling.The change simplifies the function signature while maintaining functionality by extracting the context from
__IR__. This aligns with the standardization of context handling across other modules in the codebase.This refactoring improves the module's design by:
- Reducing function signature complexity
- Standardizing how context is accessed across intrinsic functions
- Maintaining a clear separation between the public API and internal implementation details
lib/charms/simd.ex (2)
13-19: LGTM! Consider using more descriptive parameter names.The switch to
__IR__extraction is well-implemented and maintains all necessary context. However, since the parameters are unused (prefixed with_), consider naming them to match their extracted counterparts for better code readability.- defintrinsic new(_type, _literal_values) do + defintrinsic new(type_param, literal_values_param) do
46-48: LGTM! Maintain consistent parameter naming style.The
__IR__extraction is cleanly implemented. For consistency with the suggested changes in thenewfunction, consider using meaningful parameter names here as well.- defintrinsic t(_type, _width) do + defintrinsic t(type_param, width_param) dolib/charms/prelude.ex (1)
Line range hint
53-67: Consider more robust pattern matching for__IR__.The current implementation combines extraction and destructuring of
optswhich could make debugging harder if__IR__doesn't contain the expected fields. Consider splitting this into separate pattern matching steps:- opts = %Opts{args: args, ctx: ctx, block: block, loc: loc} = __IR__ + opts = __IR__ + %Opts{args: args, ctx: ctx, block: block, loc: loc} = optsThis change would:
- Make it clearer where pattern matching failures occur
- Improve error messages in case of missing fields
- Make the code more maintainable
lib/charms/pointer.ex (2)
70-76: Fix indentation for consistencyWhile the extraction of values from
__IR__is correct, the indentation of the pattern match is inconsistent with the module's style.defintrinsic element_ptr(_elem_type, _ptr, _n) do - %Opts{ - ctx: ctx, - block: block, - args: [elem_type, ptr, n] - } = __IR__ + %Opts{ + ctx: ctx, + block: block, + args: [elem_type, ptr, n] + } = __IR__
Line range hint
23-89: Consider documenting the__IR__patternThe transition to using
__IR__for context and argument extraction appears to be part of a larger refactoring effort. Consider adding documentation about this pattern in the module's documentation or in a central place, as it seems to be a fundamental change in how intrinsics are defined.lib/charms/kernel.ex (1)
89-90: LGTM! Consider improving error handling.The refactoring to use
__IR__is consistent with the binary operations. However, there are opportunities for improvement:
- Make the error message more descriptive by including the actual value:
- raise ArgumentError, "Not an integer type to negate, unsupported type: #{to_string(t)}" + raise ArgumentError, "Cannot negate value of type: #{to_string(t)}. Expected an integer type."
- Consider extracting the integer type check into a helper function since it's used in both binary and unary operations:
defp ensure_integer_type!(value, operation) do t = MLIR.Value.type(value) unless MLIR.CAPI.mlirTypeIsAInteger(t) |> Beaver.Native.to_term() do raise ArgumentError, "Cannot perform #{operation} on type: #{to_string(t)}. Expected an integer type." end t endlib/charms/intrinsic.ex (1)
Line range hint
89-97: Enhance the error message for better debuggingThe current error message could be more helpful by including information about where intrinsic functions should be used.
Consider this improvement:
- raise "Intrinsic #{Exception.format_mfa(__MODULE__, unquote(name), arity)} cannot be called outside of a defm body" + raise "Intrinsic #{Exception.format_mfa(__MODULE__, unquote(name), arity)} cannot be called outside of a defm body. " <> + "Intrinsic functions must be used within Charms.defm blocks for proper IR generation."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
lib/charms/defm/expander.ex(1 hunks)lib/charms/env.ex(1 hunks)lib/charms/intrinsic.ex(3 hunks)lib/charms/kernel.ex(2 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)
🔇 Additional comments (10)
lib/charms/simd.ex (1)
Line range hint 13-48: Verify consistent pattern usage across modules.
Let's ensure this __IR__ extraction pattern is consistently applied across related modules.
✅ Verification successful
Pattern is consistently applied across all modules
The verification shows that the __IR__ extraction pattern is consistently used across all modules in the codebase:
- All
defintrinsicfunctions follow the same pattern of extracting values from__IR__using%Opts{...} = __IR__ - The pattern is used in multiple modules including
term.ex,simd.ex,kernel.ex,prelude.ex,env.ex, andpointer.ex - The implementation in the reviewed code follows this established pattern correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of __IR__ extraction pattern in defintrinsic functions
# Search for defintrinsic function definitions and their __IR__ usage
ast-grep --pattern 'defintrinsic $name($$$) do
$$$
%Opts{$$$} = __IR__
$$$
end'
# Look for any remaining direct Opts pattern matching in function signatures
rg "defintrinsic.*%Opts.*do"
Length of output: 118
Script:
#!/bin/bash
# Let's try a different approach to find defintrinsic patterns
# Search for all defintrinsic definitions
rg "^\s*defintrinsic\s+\w+" -A 5
# Also search for __IR__ usage patterns
rg "__IR__" -A 2 -B 2
Length of output: 7045
lib/charms/prelude.ex (2)
Line range hint 30-41: LGTM! Safe implementation with proper bounds checking.
The refactoring to use __IR__ maintains the important bounds checking and detailed error messaging while simplifying the function signature.
42-44: LGTM! Clean and concise implementation.
The refactoring simplifies the function signature while maintaining the core functionality.
lib/charms/pointer.ex (2)
23-25: LGTM: Clean extraction of context and arguments from __IR__
The change aligns with the PR objective and maintains the function's logic while improving the code structure.
88-89: LGTM: Simple and correct context extraction
The change maintains the function's purpose while aligning with the new __IR__ pattern.
lib/charms/kernel.ex (1)
61-63: LGTM! Verify __IR__ availability.
The refactoring to use __IR__ improves the function signature while maintaining the same functionality. However, we should verify that __IR__ is consistently available in this macro context.
✅ Verification successful
__IR__ usage is consistent and verified
The search results confirm that __IR__ is consistently available and properly used across all defintrinsic macro contexts in the codebase:
- It's used in similar pattern matching with
%Opts{}across multiple modules (term.ex, prelude.ex, simd.ex, kernel.ex, pointer.ex, env.ex) - The macro infrastructure in
intrinsic.exexplicitly handles__IR__bindings - All usages follow the same pattern of extracting context, args, blocks, or location information
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if __IR__ is consistently used across similar contexts
# and verify its availability in macro expansions
# Search for other defintrinsic usages
rg -A 3 "defintrinsic.*do" --type elixir
# Search for __IR__ usage patterns
rg "__IR__" --type elixir
Length of output: 4272
lib/charms/intrinsic.ex (4)
98-108: LGTM! Clean implementation of IR transformation
The postwalk transformation elegantly handles the replacement of __IR__ nodes with the appropriate context reference.
113-113: LGTM! Good pattern matching for early validation
The pattern matching ensures that the required context is available early in the function execution.
83-85: Verify impact of the breaking change in function signature
The removal of the opts parameter from defintrinsic is a breaking change. Please ensure all existing usages have been updated.
38-46: Verify the availability of charms_intrinsic_internal_opts in all macro expansion contexts
The refactoring moves from explicit parameter passing to relying on a module variable. While this simplifies the interface, we should ensure that charms_intrinsic_internal_opts is always properly set before macro expansion.
Let's verify the usage:
jackalcooper
force-pushed
the
use-unhygienic-var-to-make-opts-accessible
branch
from
666634e to
b382726
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
lib/charms/prelude.ex (2)
30-39: LGTM! Consider adding documentation.The function implementation is correct and type-safe. The error handling is appropriate for out-of-bounds indices.
Consider adding
@docdocumentation to describe the function's purpose and parameters, similar to thetype_offunction.+ @doc """ + Get the result at the specified index from an MLIR operation. + Raises ArgumentError if the index is out of bounds. + """ defintrinsic result_at(%MLIR.Operation{} = op, index) do
48-52: LGTM! Consider enhancing documentation.The implementation is clean and maintains functional style using
tap.Consider enhancing the documentation to clarify the return value:
@doc """ Dump the MLIR entity at compile time with `IO.puts/1` + Returns the input entity unchanged, allowing for chaining. """lib/charms/kernel.ex (1)
89-96: Consider enhancing error handling and documentation.A few suggestions to improve the implementation:
- The error message could be more specific about supported integer types
- Consider adding documentation to explain that this implements boolean negation via xor
Here's a suggested improvement:
defintrinsic !value do t = MLIR.Value.type(value) unless MLIR.CAPI.mlirTypeIsAInteger(t) |> Beaver.Native.to_term() do - raise ArgumentError, "Not an integer type to negate, unsupported type: #{to_string(t)}" + raise ArgumentError, "Boolean negation requires an integer type (i1, i8, etc), got: #{to_string(t)}" end quote bind_quoted: [v: value, t: t] do one = const 1 :: t value arith.xori(v, one) :: t # Implements boolean negation: result = value XOR 1 end endtest/defm_test.exs (1)
208-220: Consider strengthening the test assertions.The test could be more robust with these improvements:
- Use a more specific regex pattern to avoid false positives
- Add assertions to verify the function's return value
Here's a suggested enhancement:
test "dump" do + test_value = 42 assert capture_io(fn -> - defmodule DumpTerm do - use Charms - alias Charms.Term - - defm d(env, a) :: Term.t() do - dump(a) - func.return(a) - end - end - end) =~ ~r"block argument.+i64" + defmodule DumpTerm do + use Charms + alias Charms.Term + + defm d(env, a) :: Term.t() do + dump(a) + func.return(a) + end + end + end) =~ ~r"block argument 'a' of type 'i64'" + + assert DumpTerm.d(test_value) == test_value end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
lib/charms/defm/expander.ex(5 hunks)lib/charms/env.ex(1 hunks)lib/charms/intrinsic.ex(4 hunks)lib/charms/kernel.ex(2 hunks)lib/charms/pointer.ex(2 hunks)lib/charms/prelude.ex(2 hunks)lib/charms/simd.ex(2 hunks)lib/charms/term.ex(1 hunks)test/defm_test.exs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/charms/defm/expander.ex
- lib/charms/env.ex
- lib/charms/intrinsic.ex
- lib/charms/simd.ex
- lib/charms/term.ex
🔇 Additional comments (12)
lib/charms/pointer.ex (6)
15-16: LGTM! Clean implementation using bind_quoted
The change improves code clarity by explicitly binding the elem_type variable in the quoted expression.
52-53: LGTM! Simplified implementation
The change maintains type safety while making the code more concise using bind_quoted.
61-62: LGTM! Consistent with the load implementation
The change follows the same clean pattern as the load function, maintaining consistency in the codebase.
83-86: LGTM! Clean type creation
The t/0 function correctly creates a deferred LLVM pointer type using the context from __IR__.
23-44: Verify type compatibility across MLIR operations
The implementation correctly handles size type conversion to i64, but we should verify that this type is consistently used across other MLIR operations in the codebase.
✅ Verification successful
Let me gather more information about type handling across MLIR operations.
Type handling for size operations is consistent across the codebase
The verification shows that:
- All size-related operations consistently use i64 type casting through
index.castsorarith.extsi - Type checking with
MLIR.Value.typeis properly implemented across the codebase - The
allocate/2function's type handling aligns with other size-related operations inlib/charms/defm/expander.exand test files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other MLIR operations that might need similar i64 type handling
ast-grep --pattern 'value arith.extsi($_) :: i64()'
Length of output: 125
Script:
#!/bin/bash
# Search for i64 type usage in MLIR operations
rg ":: i64\(\)" -A 2
# Search for other size-related operations
rg "size.*::" -A 2
# Look for MLIR.Value.type usage
ast-grep --pattern 'MLIR.Value.type($_)'
Length of output: 2362
Line range hint 69-77: Verify the hardcoded constant index value
While the MLIR DSL usage is correct, the hardcoded value -2147483648 in rawConstantIndices needs verification:
- Is this value intentional?
- Should it be parameterized or documented?
lib/charms/prelude.ex (2)
41-46: LGTM! Well-documented and clean implementation.
The function is well-documented and follows the single responsibility principle.
61-64: LGTM! Verify intrinsic function generation.
The changes correctly utilize __IR__ for context and maintain existing functionality while improving code clarity.
Let's verify that all intrinsic functions are properly generated:
✅ Verification successful
LGTM! Intrinsic function generation is correctly implemented.
The verification shows that:
- The
defintrinsicmacro is properly used across multiple modules with consistent__IR__pattern matching - In
prelude.ex, the dynamic generation of intrinsic functions from@enif_functionscorrectly:- Uses
__IR__to extract context, block, and location - Maintains proper signature handling with
Beaver.ENIF.signature - Wraps arguments consistently using the
wrap_arghelper - Generates the appropriate MLIR function calls
- Uses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all functions in @enif_functions are properly defined
# and that their signatures match the expected pattern using __IR__.
# Search for all defintrinsic function definitions
ast-grep --pattern 'defintrinsic $name($$$) do
opts = %Opts{$$$} = __IR__
$$$
end'
Length of output: 88
Script:
#!/bin/bash
# Let's try a different approach to verify the intrinsic function definitions
# First, let's check the content of the prelude.ex file
cat lib/charms/prelude.ex
# As a backup, let's also search for any files containing 'defintrinsic'
rg "defintrinsic" -A 5
Length of output: 8225
lib/charms/kernel.ex (2)
89-90: LGTM! Clean signature refactoring.
The simplified function signature improves readability while maintaining the same functionality.
61-63: LGTM! Clean refactoring to use __IR__.
The change simplifies the function signature while maintaining the same functionality by extracting context from __IR__. This is a good improvement in code clarity.
Let's verify the macro expansion:
✅ Verification successful
Verified: __IR__ macro expansion is correctly implemented
The code changes are safe and correct. The defintrinsic macro properly handles __IR__ by transforming it into var!(charms_intrinsic_internal_opts) during macro expansion, which maintains the same functionality as the previous implementation but with cleaner syntax. The macro ensures that the opts struct with all necessary context (ctx, block, loc) is available to intrinsic implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that __IR__ is properly defined in the macro context
ast-grep --pattern 'defmacro defintrinsic($_, $_) do
$$$
__IR__
$$$
end'
Length of output: 80
Script:
#!/bin/bash
# Let's try a broader search for defintrinsic macro definition
ast-grep --pattern 'defmacro defintrinsic'
# Also search for any __IR__ usage in the file
rg "__IR__" --type elixir -C 3
Length of output: 3304
Script:
#!/bin/bash
# Let's check the defintrinsic macro definition in intrinsic.ex
rg "defmacro defintrinsic" -A 10 lib/charms/intrinsic.ex
Length of output: 510
Script:
#!/bin/bash
# Let's see the complete defintrinsic macro implementation to understand how __IR__ is handled
rg "defmacro defintrinsic" -A 30 lib/charms/intrinsic.ex
# Also check for any opts variable setup
rg "var!\(charms_intrinsic_internal_opts\)" -A 5 lib/charms/intrinsic.ex
Length of output: 1641
test/defm_test.exs (2)
44-44: LGTM!
The import statement is correctly placed and necessary for the new test case.
208-220: LGTM! Test structure and implementation are sound.
The test case correctly verifies the output of the dump/1 macro while maintaining proper module structure and function implementation.
Summary by CodeRabbit
New Features
dumpfunction for outputting MLIR entities at compile time.Bug Fixes
Refactor
These changes enhance the clarity, robustness, and usability of the module, ensuring a smoother experience for end-users.