Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass to check poison op #33

Merged
merged 2 commits into from
Oct 5, 2024
Merged

Pass to check poison op #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
21e1de0
add pass to check poison op
jackalcooper Oct 5, 2024
f408527
Update call_test.exs
jackalcooper Oct 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions lib/charms/defm.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,28 @@ defmodule Charms.Defm do
end
end

defp check_poison!(op) do
Beaver.Walker.postwalk(op, fn
%MLIR.Operation{} = op ->
if MLIR.Operation.name(op) == "ub.poison" do
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
Comment on lines +138 to +144
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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

else
op
end

ir ->
ir
end)

:ok
end

@doc false
def compile_definitions(definitions) do
import MLIR.Transforms
Expand Down Expand Up @@ -161,6 +183,7 @@ defmodule Charms.Defm do
m
|> Charms.Debug.print_ir_pass()
|> MLIR.Pass.Composer.nested("func.func", Charms.Defm.Pass.CreateAbsentFunc)
|> MLIR.Pass.Composer.append({"check-poison", "builtin.module", &check_poison!/1})
|> canonicalize
|> MLIR.Pass.Composer.run!(print: Charms.Debug.step_print?())
|> MLIR.to_string(bytecode: true)
Expand Down
26 changes: 26 additions & 0 deletions test/call_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
defmodule CallTest do
use ExUnit.Case, async: true
import ExUnit.CaptureLog

test "if with value" do
line = __ENV__.line + 10

log =
capture_log(fn ->
assert_raise RuntimeError, fn ->
defmodule CallingAbsentFunc do
use Charms
alias Charms.{Pointer, Term}

defm get(env, i) :: Term.t() do
AbsentMod.absent_fun(env, i)
func.return(i)
end
Comment on lines +15 to +18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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)
 end

This 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.

Suggested change
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

end
end
end)

assert log =~ "Unknown intrinsic: AbsentMod.absent_fun/2"
assert log =~ "#{__ENV__.file}:#{line}"
end
end
Loading