Pass to check poison op #33
Conversation
WalkthroughThe changes in this pull request introduce a new private function, 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: 2
🧹 Outside diff range and nitpick comments (2)
test/call_test.exs (1)
23-25: LGTM: Assertions are well-structured, with a minor suggestion.The assertions correctly check for the expected error messages in the log output. The use of
__ENV__.fileand the pre-calculatedlinevariable is a good practice for ensuring the correct file and line number are reported.Consider using a more specific assertion for the error message:
assert log =~ ~r/Unknown intrinsic: AbsentMod\.absent_fun\/2/This regex-based assertion would be more robust against minor changes in the error message format.
lib/charms/defm.ex (1)
186-186: Consider the placement of thecheck_poison!/1pass in the pipelineCurrently, the
check_poison!/1pass is appended before thecanonicalizepass. It might be beneficial to runcanonicalizefirst to simplify the IR, which could potentially eliminate some "ub.poison" operations before the poison check. Placing the poison check after canonicalization might improve efficiency and reduce unnecessary exceptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- lib/charms/defm.ex (2 hunks)
- test/call_test.exs (1 hunks)
🔇 Additional comments (3)
test/call_test.exs (2)
1-7: LGTM: Test setup is well-structured.The test module is correctly set up using ExUnit with async testing enabled. The import of
ExUnit.CaptureLogis appropriate for the test's purpose of capturing log output.
8-21: Verify: Unusual test structure with inline module definition.The test case has an unconventional structure where a module
CallingAbsentFuncis defined within the test itself. While this is valid Elixir code, it's an unusual pattern that might affect test isolation and readability.Questions to consider:
- Is this inline module definition necessary for the test case?
- Could this module be defined outside the test or in a separate file for better organization?
The use of
Charmsand thedefmmacro suggests this is testing charm-related functionality. Please ensure this structure is intentional and necessary for the specific testing requirements.lib/charms/defm.ex (1)
134-154: Functioncheck_poison!/1is well-implementedThe
check_poison!/1function effectively traverses the MLIR operation to detect any "ub.poison" operations and raises an informative error message when one is found. The use ofpostwalkand pattern matching on%MLIR.Operation{}is appropriate, and the function adheres to Elixir conventions.
| defm get(env, i) :: Term.t() do | ||
| AbsentMod.absent_fun(env, i) | ||
| func.return(i) | ||
| end |
There was a problem hiding this comment.
Remove unreachable code in the get/2 function.
The get/2 function contains unreachable code. The line func.return(i) will never be executed because AbsentMod.absent_fun(env, i) is expected to raise an error.
Consider removing the unreachable code:
defm get(env, i) :: Term.t() do
AbsentMod.absent_fun(env, i)
- func.return(i)
endThis change will make the function's intent clearer without affecting the test's behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defm get(env, i) :: Term.t() do | |
| AbsentMod.absent_fun(env, i) | |
| func.return(i) | |
| end | |
| defm get(env, i) :: Term.t() do | |
| AbsentMod.absent_fun(env, i) | |
| end |
| if msg = Beaver.Walker.attributes(op)["msg"] do | ||
| msg = MLIR.CAPI.mlirStringAttrGetValue(msg) |> MLIR.StringRef.to_string() | ||
| msg <> ", " <> to_string(MLIR.Operation.location(op)) | ||
| else | ||
| "Poison operation detected in the IR. #{to_string(op)}" | ||
| end | ||
| |> raise |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider defining a custom exception for poison operations
Currently, the code raises a generic exception with a custom message when a poison operation is detected. Defining a custom exception, such as PoisonOperationError, would make error handling more explicit and allow for more precise exception matching downstream.
Example:
defmodule PoisonOperationError do
defexception message: "Poison operation detected in the IR."
end
# Then, in your `check_poison!/1` function:
...
msg = ...
raise PoisonOperationError, message: msg
Summary by CodeRabbit
New Features
Bug Fixes