-
Notifications
You must be signed in to change notification settings - Fork 18
Accept :hooks compiler opt in EMLX.__compile__/4 #133
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.
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1741,7 +1741,16 @@ defmodule EMLX do | |
| # Known EMLX-specific compiler opts. `:command_queue` is injected by | ||
| # `__partitions_options__/1` but may also be passed directly by callers | ||
| # that manage their own queues (equivalent to a manual `with_queue`). | ||
| @valid_compiler_keys [:device, :max_concurrency, :command_queue] | ||
| # | ||
| # `:hooks` isn't EMLX-specific — `Nx.Defn.compile/3`/`jit/2` always forward | ||
| # it (defaulting to `%{}`) to whichever compiler is selected (see | ||
| # `Nx.Defn.prepare_options/1`), so every `Nx.Defn.Compiler` callback must | ||
| # accept it even if unused. `:hook`/`:io_call` *expr nodes* (each carrying | ||
| # their own inline default callback) do lower natively — see | ||
| # `EMLX.Native.Expr`'s `:hook`/`:io_call` clause — but the *named-override* | ||
| # map this option carries (swapping in a different callback per hook name | ||
| # at compile time) isn't wired through, so it's only accepted when empty. | ||
| @valid_compiler_keys [:device, :max_concurrency, :command_queue, :hooks] | ||
|
|
||
| # Process-lifetime dispatch cache backing `dispatch_key/3` + | ||
| # `get_or_compile_program/6` (see their docs) — a compiled program is keyed | ||
|
|
@@ -1772,6 +1781,19 @@ defmodule EMLX do | |
| @impl Nx.Defn.Compiler | ||
| def __compile__(_key, vars, fun, opts) do | ||
| Keyword.validate!(opts, @valid_compiler_keys) | ||
|
|
||
| case Keyword.get(opts, :hooks, %{}) do | ||
| empty when empty == %{} -> | ||
| :ok | ||
|
|
||
| hooks -> | ||
| raise ArgumentError, | ||
| "EMLX does not support the :hooks named-override map (got callbacks for " <> | ||
| "#{inspect(Map.keys(hooks))}) — :hook/:io_call expr nodes lower natively " <> | ||
| "with their own inline default callback, but swapping in a different " <> | ||
| "callback per hook name via compiler opts isn't wired through" | ||
| end | ||
|
Comment on lines
+1776
to
+1786
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe we should add support for them. Can you either implement this (bigger ask) or open a related issue?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can open an issue for this now. I can pick it up later.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good! |
||
|
|
||
| queue = Keyword.get(opts, :command_queue) | ||
| device = Keyword.get(opts, :device, default_device()) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove these comments