Skip to content

Commit 5a0ee2f

Browse files
committed
refactor: address CM2 code-review findings
- Raise on unhandled block structs instead of lowering their composed expansion. For blocks Emily.Backend.block/4 dispatches through a fused / native kernel (SDPAWithSinks, the Nx.Block.LinAlg.* / Take / FFT / cumulative families) the composed path would silently diverge from the Evaluator; a clear "unsupported" is the honest no-fallback behaviour. More fused blocks land with their opcodes. - Gate the affine packed-shape output-feature formula to mode "affine" in quantized_matmul_defn; other layouts derive the feature count from the dequantized shape. - Add Emily.IR.emit_coerced/5 and route the lower_block clauses through it (removes the emit-then-coerce footgun). - C++: fold scalar_attr into scalar_at (one implementation); fix the stale qmode_from_code doc comment.
1 parent 8a1027a commit 5a0ee2f

3 files changed

Lines changed: 41 additions & 32 deletions

File tree

c_src/emily/opcodes.hpp

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ enum class Opcode : int64_t {
110110

111111
inline constexpr int64_t kOpcodeCount = 61;
112112

113-
// Quant mode code (Emily.IR.mode_code/1) -> MLX mode string.
113+
// Quant mode code (Emily.IR @quant_modes) -> MLX mode string.
114114
inline std::string qmode_from_code(int64_t code) {
115115
switch (code) {
116116
case 0: return "affine";
@@ -160,15 +160,6 @@ inline const std::vector<int64_t> &attr0(const std::vector<std::vector<int64_t>>
160160
return a[0];
161161
}
162162

163-
inline int64_t scalar_attr(const std::vector<std::vector<int64_t>> &a,
164-
const char *name) {
165-
const auto &v = attr0(a, name);
166-
if (v.size() != 1) {
167-
throw std::invalid_argument(std::string(name) + " expects one attribute value");
168-
}
169-
return v[0];
170-
}
171-
172163
inline const std::vector<int64_t> &
173164
attr_at(const std::vector<std::vector<int64_t>> &a, std::size_t i,
174165
const char *name) {
@@ -314,7 +305,7 @@ inline mx::array dispatch_op(Opcode op, const std::vector<mx::array> &in,
314305
// --- Cast / shape ---
315306
case Opcode::Astype:
316307
return mx::astype(arg1(in, "astype"),
317-
emily::to_mlx_dtype_code(scalar_attr(iattrs, "astype")), s);
308+
emily::to_mlx_dtype_code(scalar_at(iattrs, 0, "astype")), s);
318309
case Opcode::Reshape:
319310
return mx::reshape(arg1(in, "reshape"),
320311
emily::to_mlx_shape(attr0(iattrs, "reshape")), s);
@@ -374,7 +365,7 @@ inline mx::array dispatch_op(Opcode op, const std::vector<mx::array> &in,
374365
std::to_string(in.size()));
375366
}
376367
auto eps = static_cast<float>(
377-
emily::f64_from_bits(scalar_attr(iattrs, "fast_rms_norm")));
368+
emily::f64_from_bits(scalar_at(iattrs, 0, "fast_rms_norm")));
378369
return mx::fast::rms_norm(in[0], std::optional<mx::array>(in[1]), eps, s);
379370
}
380371
case Opcode::FastLayerNorm: {
@@ -383,7 +374,7 @@ inline mx::array dispatch_op(Opcode op, const std::vector<mx::array> &in,
383374
std::to_string(in.size()));
384375
}
385376
auto eps = static_cast<float>(
386-
emily::f64_from_bits(scalar_attr(iattrs, "fast_layer_norm")));
377+
emily::f64_from_bits(scalar_at(iattrs, 0, "fast_layer_norm")));
387378
return mx::fast::layer_norm(in[0], std::optional<mx::array>(in[1]),
388379
std::optional<mx::array>(in[2]), eps, s);
389380
}

lib/emily/ir.ex

Lines changed: 33 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -501,25 +501,22 @@ defmodule Emily.IR do
501501
defp lower_block(%FB.RMSNorm{eps: eps}, [x, weight], _expr, t, state) do
502502
{rx, state} = lower_node(x, state)
503503
{rw, state} = lower_node(weight, state)
504-
{r, state} = emit(state, :fast_rms_norm, [rx, rw], [[float_bits(eps)]])
505-
coerce(r, t.type, state)
504+
emit_coerced(state, :fast_rms_norm, [rx, rw], [[float_bits(eps)]], t.type)
506505
end
507506

508507
defp lower_block(%FB.LayerNorm{eps: eps}, [x, weight, bias], _expr, t, state) do
509508
{rx, state} = lower_node(x, state)
510509
{rw, state} = lower_node(weight, state)
511510
{rb, state} = lower_node(bias, state)
512-
{r, state} = emit(state, :fast_layer_norm, [rx, rw, rb], [[float_bits(eps)]])
513-
coerce(r, t.type, state)
511+
emit_coerced(state, :fast_layer_norm, [rx, rw, rb], [[float_bits(eps)]], t.type)
514512
end
515513

516514
defp lower_block(%FB.RoPE{} = b, [x, offset], _expr, t, state) do
517515
{rx, state} = lower_node(x, state)
518516
{ro, state} = lower_node(offset, state)
519517

520518
attrs = [[b.dims], [bool_int(b.traditional)], [float_bits(b.base)], [float_bits(b.scale)]]
521-
{r, state} = emit(state, :fast_rope, [rx, ro], attrs)
522-
coerce(r, t.type, state)
519+
emit_coerced(state, :fast_rope, [rx, ro], attrs, t.type)
523520
end
524521

525522
defp lower_block(%FB.RoPEWithFreqs{} = b, [x, offset, freqs], _expr, t, state) do
@@ -528,25 +525,29 @@ defmodule Emily.IR do
528525
{rf, state} = lower_node(freqs, state)
529526

530527
attrs = [[b.dims], [bool_int(b.traditional)], [float_bits(b.scale)]]
531-
{r, state} = emit(state, :fast_rope_freqs, [rx, ro, rf], attrs)
532-
coerce(r, t.type, state)
528+
emit_coerced(state, :fast_rope_freqs, [rx, ro, rf], attrs, t.type)
533529
end
534530

535531
defp lower_block(%FB.SDPA{scale: scale, causal: causal}, [q, k, v], _expr, t, state) do
536532
{rq, state} = lower_node(q, state)
537533
{rk, state} = lower_node(k, state)
538534
{rv, state} = lower_node(v, state)
539-
{r, state} = emit(state, :fast_sdpa, [rq, rk, rv], [[float_bits(scale)], [bool_int(causal)]])
540-
coerce(r, t.type, state)
535+
536+
emit_coerced(
537+
state,
538+
:fast_sdpa,
539+
[rq, rk, rv],
540+
[[float_bits(scale)], [bool_int(causal)]],
541+
t.type
542+
)
541543
end
542544

543545
defp lower_block(%FB.SDPAWithMask{scale: scale}, [q, k, v, mask], _expr, t, state) do
544546
{rq, state} = lower_node(q, state)
545547
{rk, state} = lower_node(k, state)
546548
{rv, state} = lower_node(v, state)
547549
{rm, state} = lower_node(mask, state)
548-
{r, state} = emit(state, :fast_sdpa_mask, [rq, rk, rv, rm], [[float_bits(scale)]])
549-
coerce(r, t.type, state)
550+
emit_coerced(state, :fast_sdpa_mask, [rq, rk, rv, rm], [[float_bits(scale)]], t.type)
550551
end
551552

552553
defp lower_block(%QB.QuantizedMatmul{} = qb, [x, q, s, b], _expr, t, state) do
@@ -562,14 +563,20 @@ defmodule Emily.IR do
562563
[Map.fetch!(@quant_modes, qb.mode)]
563564
]
564565

565-
{r, state} = emit(state, :quantized_matmul, [rx, rq, rs, rb], attrs)
566-
coerce(r, t.type, state)
566+
emit_coerced(state, :quantized_matmul, [rx, rq, rs, rb], attrs, t.type)
567567
end
568568

569-
# Unknown block struct: lower its pre-composed default expansion (the
570-
# `expr` arg) instead of the fused kernel — no runtime fallback.
571-
defp lower_block(_struct, _in_args, expr, _t, state) do
572-
lower_node(expr, state)
569+
# Any other block struct raises. Lowering the block's composed
570+
# expansion would silently diverge from the Evaluator whenever
571+
# Emily.Backend.block/4 dispatches that struct through a fused / native
572+
# kernel (e.g. SDPAWithSinks, the Nx.Block.LinAlg.* / Take / FFT /
573+
# cumulative families) — a worse failure than a clear "unsupported".
574+
# Additional fused blocks are added alongside their opcode.
575+
defp lower_block(struct, _in_args, _expr, _t, _state) do
576+
raise ArgumentError,
577+
"Emily Expr compiler does not yet lower the block " <>
578+
"#{inspect(struct.__struct__)} (no fallback). Supported: RMSNorm, " <>
579+
"LayerNorm, RoPE, RoPEWithFreqs, SDPA, SDPAWithMask, QuantizedMatmul."
573580
end
574581

575582
defp bool_int(true), do: 1
@@ -592,6 +599,14 @@ defmodule Emily.IR do
592599
{ref, %{state | instrs: [instr | state.instrs], n_instrs: state.n_instrs + 1}}
593600
end
594601

602+
# Emit an instruction then coerce its output to `type` (mirrors
603+
# Emily.Backend.wrap/3). The trailing coerce is mandatory on every
604+
# value-producing op, so the helper keeps the per-clause tail honest.
605+
defp emit_coerced(state, opcode, operands, iattrs, type) do
606+
{r, state} = emit(state, opcode, operands, iattrs)
607+
coerce(r, type, state)
608+
end
609+
595610
# Materialize an Nx tensor (already on a host backend) as a captured
596611
# const / weight ref, held by the program for its lifetime. `:const`
597612
# holds materialized literal constants (and iota); `:capture` holds

lib/emily/quantization.ex

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,12 +171,15 @@ defmodule Emily.Quantization do
171171
# bits ∈ {2,4,8} the packed last axis unpacks by 32/bits.
172172
defp qmm_out_features(%QuantizedWeight{value: value}, true), do: elem(Nx.shape(value), 0)
173173

174-
defp qmm_out_features(%QuantizedWeight{value: value, bits: bits}, false)
174+
defp qmm_out_features(%QuantizedWeight{value: value, bits: bits, mode: "affine"}, false)
175175
when bits in [2, 4, 8] do
176+
# Affine packs `32 / bits` lanes per u32 along the last axis.
176177
shape = Nx.shape(value)
177178
elem(shape, tuple_size(shape) - 1) * div(32, bits)
178179
end
179180

181+
# Other layouts (microscaled, or affine bits 3/6): derive the output
182+
# feature count from the dequantized weight's shape.
180183
defp qmm_out_features(qw, false) do
181184
shape = Nx.shape(dequantize_defn(qw))
182185
elem(shape, tuple_size(shape) - 1)

0 commit comments

Comments
 (0)