Skip to content

Commit 9a771e4

Browse files
YWHyukclaude
andcommitted
[Frontend] Fix #238: replace var_info dict with typed CSEVariable attributes
Issue #238 was the visible symptom (silent bool/uint8 -> int8 downcast via the lossy `MLIR_TO_DTYPE[var_info[1]]` round-trip at mlir_codegen_backend.py:1535) of a deeper architectural smell: PyTorchSim maintained a parallel `self.var_info` dict tracking `[vec_size, mlir_dtype_string]` per csevar, duplicating type info that already lives on Inductor's `CSEVariable.dtype`. The lossy MLIR->torch round-trip was the only place this duplication actively caused corruption, but collapsing the two systems is the structural fix. Core changes: - New type: - `MLIRCSEVariable(common.CSEVariable)` carries `vec_size: int` and inherits `dtype: Optional[torch.dtype]`. `mlir_dtype` is a derived @Property from `dtype` via `DTYPE_TO_MLIR`. There is no separate predicate/mask subclass: `torch.bool` maps to MLIR `"i1"` directly (DTYPE_TO_MLIR[torch.bool] = "i1"). MLIR-to-LLVM lowering pads i1 storage to bytes, matching the wrapper C ABI (`uint8_t*`, one byte per element). The wrapper architecturally cannot accept bit-packed i1 storage (mlir_caller_codegen.py uses sizeof(ctype) loads), so the `memref<...xi1>` -> `i8`-backed pipeline is the natural fit. - `OpResult(vec_size, dtype)` frozen dataclass replaces the legacy `[vec_size, mlir_dtype_string]` ret_info list. `OpResult.from_var` and `OpResult.from_mlir` are classmethod constructors. - `INDEX_DTYPE` singleton sentinel for MLIR `index` type (no torch equivalent). `MLIR_TO_DTYPE["index"] = INDEX_DTYPE` and `DTYPE_TO_MLIR[INDEX_DTYPE] = "index"` so the dicts are bijective for all known types — clearer than overloading `None`. - `MLIRCSE(common.CSE)` extends Inductor's CSE with a `vec_size` axis: - `newvar` / `namedvar` construct `MLIRCSEVariable` directly, bypassing the kernel-side `V.kernel.create_cse_var` hook (which is no longer needed). - `generate(buffer, code, *, vec_size=N, dtype=X, ...)` plumbs `vec_size` to `newvar` via a transient instance attribute, calling `super().generate(...)` for the rest. No need to reimplement the upstream generate body. - Handler proxy (mlir_common.py CSEProxy) rewritten to expect `(code, OpResult|None)` from ops. Single uniform path: `target_cse.generate(buf, code, dtype=ret.dtype, vec_size=ret.vec_size)` — no post-hoc attribute assignment. - All ops in mlir_ops.py, mlir_template.py, mlir_sort_template.py return `(code, OpResult)` (or `OpResult.from_var` / `OpResult.from_mlir` helpers). Legacy `[size, mlir_str]` shape gone. - `register_var_info` / `register_var_cse` deleted. Six previously-named csevars (`compute_idx`, `itervar_cses`, `init_iter`, `reduce_loop_idx`, `idx_step_index`, `idx_base`) now use `cse.namedvar(..., dtype=..., vec_size=...)` directly. `make_named_csevar` wrapper removed. - ~108 read sites of `var_info[v][...]` migrated to attribute access (`v.vec_size`, `v.mlir_dtype`). `var_info[v][1] == "i1"` patterns collapse to `v.dtype == torch.bool` since the mask subclass is gone. - `self.var_info` dict removed entirely. - Issue #238 fix at mlir_codegen_backend.py:1535: csevar = self.cse.varname_map[target_dim] dtype = csevar.dtype No more round-trip; the torch dtype set at csevar construction is preserved end-to-end. Files touched: mlir_common.py (foundation), mlir_codegen_backend.py (#238 site + read migration + memory-entry call sites), mlir_ops.py (ops layer ret_info migration), mlir_template.py + mlir_sort_template.py (template ops + named csevar sites). Sample-verified: test_add, test_softmax, test_sort (i1 mask path via cmp), test_matmul, test_layernorm, test_indirect_access (#238 critical path), test_expert_mask, test_transcendental, test_reduce. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 8306055 commit 9a771e4

5 files changed

Lines changed: 384 additions & 320 deletions

File tree

PyTorchSimFrontend/mlir/mlir_codegen_backend.py

Lines changed: 49 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -295,17 +295,17 @@ def __init__(self, kernel_group, reason=None):
295295
self.header.writeline(" return p;")
296296
self.header.writeline("}")
297297
self.header.writeline("void __wrap_free(void *ptr) { return; }")
298-
self.reduction_cse = common.CSE(self.newvar_prefix, self.suffix, name_prefix="tmp_acc")
299-
self.spad_cse = common.CSE(self.newvar_prefix, self.suffix, name_prefix="spad")
300-
self.apply_cse = common.CSE(self.newvar_prefix, self.suffix, name_prefix="apply")
301-
self.mask_cse = common.CSE(self.newvar_prefix, self.suffix, name_prefix="mask")
302-
self.iterator_cse = common.CSE(self.newvar_prefix, self.suffix, name_prefix="iter")
303-
self.init_cse = common.CSE(self.newvar_prefix, self.suffix, name_prefix="init")
304-
self.init_vec_cse = common.CSE(self.newvar_prefix, self.suffix, name_prefix="init_vec")
305-
self.const_cse = common.CSE(self.newvar_prefix, self.suffix, name_prefix="const")
306-
self.alloc_cse = common.CSE(self.newvar_prefix, self.suffix, name_prefix="alloc")
307-
self.indexed_cse = common.CSE(self.newvar_prefix, self.suffix, name_prefix="indexed_op")
308-
self.map_cse = common.CSE("#", self.suffix, name_prefix="map")
298+
self.reduction_cse = mlir_common.MLIRCSE(self.newvar_prefix, self.suffix, name_prefix="tmp_acc")
299+
self.spad_cse = mlir_common.MLIRCSE(self.newvar_prefix, self.suffix, name_prefix="spad")
300+
self.apply_cse = mlir_common.MLIRCSE(self.newvar_prefix, self.suffix, name_prefix="apply")
301+
self.mask_cse = mlir_common.MLIRCSE(self.newvar_prefix, self.suffix, name_prefix="mask")
302+
self.iterator_cse = mlir_common.MLIRCSE(self.newvar_prefix, self.suffix, name_prefix="iter")
303+
self.init_cse = mlir_common.MLIRCSE(self.newvar_prefix, self.suffix, name_prefix="init")
304+
self.init_vec_cse = mlir_common.MLIRCSE(self.newvar_prefix, self.suffix, name_prefix="init_vec")
305+
self.const_cse = mlir_common.MLIRCSE(self.newvar_prefix, self.suffix, name_prefix="const")
306+
self.alloc_cse = mlir_common.MLIRCSE(self.newvar_prefix, self.suffix, name_prefix="alloc")
307+
self.indexed_cse = mlir_common.MLIRCSE(self.newvar_prefix, self.suffix, name_prefix="indexed_op")
308+
self.map_cse = mlir_common.MLIRCSE("#", self.suffix, name_prefix="map")
309309
self.global_vars_dict = dict()
310310
self.reduction_vars = dict()
311311
self.consts = dict()
@@ -549,7 +549,12 @@ def load(self, name: str, index: sympy.Expr):
549549
else:
550550
# FIXME. Any good idea?
551551
out = sram_var
552-
self.register_var_info(out, [compute_vec_size, mlir_dtype])
552+
# `out` is the spad memref reference (an MLIRCSEVariable from
553+
# spad_cse.generate). Annotate it with the load's compute-vec
554+
# size and torch dtype so downstream attribute reads (vec_size,
555+
# mlir_dtype) reflect the load shape.
556+
out.vec_size = compute_vec_size
557+
out.dtype = dtype
553558
self.spad_buffer_dict[str(out)] = [sram_var, local_tile_desc.get_tile_size(), tile_numel_per_lane, sram_index_var, tile_shape, vshape]
554559
return out
555560

@@ -593,11 +598,11 @@ def store(self, name: str, index: sympy.Expr, value, mode=None, *args, **kwargs)
593598
sram_var, sram_index_var = self.get_scratchpad_buffer(dtype, name, local_tile_desc, index)
594599
compute_index_var = ",".join(sram_index_var.split(",")[:-1] + [f"%{self.compute_idx}"])
595600
# Generate vector store instruction
596-
_, operand_type = self.var_info[value]
601+
_, operand_type = value.vec_size, value.mlir_dtype
597602
if mlir_dtype != operand_type:
598603
value = ops.to_dtype(value, mlir_dtype)
599604

600-
if compute_vec_size < self.var_info[value][0]:
605+
if compute_vec_size < value.vec_size:
601606
with self.override_buffer_cse(buffer=self.stores):
602607
value = ops.extract_strided_slice(value, compute_vec_size)
603608

@@ -644,14 +649,20 @@ def reduction(self, dtype, src_dtype, reduction_type, value):
644649
init = self.get_const_cse(reduction_init(reduction_type, dtype), type_name)
645650
init_vec = init if vec_len == 1 else ops.broadcast(init, vec_len)
646651

652+
# The outermost acc (reduction_depth == 0) carries the final reduced
653+
# shape; inner accumulators stay at default vec_size=1 until lowered.
654+
outer_reduction_size = self.kernel_group.tile_desc.get_numel_per_lane() // self.kernel_group.tile_desc.get_reduction_numel()
647655
acc_var_list = []
648656
iter_var_list = []
649657
for reduction_depth in range(self.get_nr_rdim()):
650658
# Create reduction key
651659
reduction_key = src_dtype, reduction_type, value, reduction_depth
652660
acc_init_var = init_vec if reduction_depth == 0 else iter_var_list[-1]
653661

654-
acc = self.reduction_cse.generate(self.loads, f"reduction {reduction_key}", write=False)
662+
acc = self.reduction_cse.generate(
663+
self.loads, f"reduction {reduction_key}",
664+
write=False, dtype=dtype, vec_size=outer_reduction_size,
665+
)
655666
iterator = self.iterator_cse.generate(self.loads, f"reduction {reduction_key}", write=False)
656667
acc_var_list.append(acc)
657668
iter_var_list.append(iterator)
@@ -664,8 +675,10 @@ def reduction(self, dtype, src_dtype, reduction_type, value):
664675
# Note: reduction body is inner most loop body. So it doesn't need reduction depth.
665676
body_key = src_dtype, reduction_type, value
666677
body_acc = self.reduction_cse.generate(self.compute, f"reduction {body_key}body_acc", write=False)
667-
body_iter_arg = self.iterator_cse.generate(self.compute, f"reduction {body_key}body_iter_arg", write=False)
668-
self.register_var_info(body_iter_arg, [vec_len, type_name])
678+
body_iter_arg = self.iterator_cse.generate(
679+
self.compute, f"reduction {body_key}body_iter_arg",
680+
write=False, dtype=dtype, vec_size=vec_len,
681+
)
669682
acc_var_list.append(body_acc)
670683

671684
# Reduction body codegen
@@ -683,9 +696,8 @@ def reduction(self, dtype, src_dtype, reduction_type, value):
683696
self.affine_yield[acc] = reduced_shape, reduction_depth
684697

685698
# Final reduction
686-
reduction_size = self.kernel_group.tile_desc.get_numel_per_lane() // self.kernel_group.tile_desc.get_reduction_numel()
687-
acc = acc_var_list[0] # Set outermost acc var
688-
self.register_var_info(acc, [reduction_size, type_name])
699+
reduction_size = outer_reduction_size # already attached to acc_var_list[0]
700+
acc = acc_var_list[0] # outermost acc var (already typed at creation)
689701
assert(vec_len % reduction_size==0)
690702

691703
# Prepare init value
@@ -794,7 +806,7 @@ def _index_expr(self, tile_desc, renamed_expression, index, base_vector_index):
794806

795807
with self.override_buffer_cse(buffer=self.const_buffer, cse=self.const_cse):
796808
vlane_offset = ops.vlane_offset(vlane_vec, vlane_vec, attributes={"vlane_offset": offset}, comment="vlane offset")
797-
if compute_vec_size < self.var_info[vlane_offset][0]:
809+
if compute_vec_size < vlane_offset.vec_size:
798810
vlane_offset = ops.extract_strided_slice(vlane_offset, compute_vec_size)
799811
vlane_offset = ops.index_cast(vlane_offset, "index")
800812
dim = ops.add(dim, vlane_offset)
@@ -874,7 +886,7 @@ def index_expr(self, index, dtype):
874886

875887
# Initialize base vector
876888
if not self.base_vector_initialized:
877-
init_iter = self.register_var_cse("init_iter", 1, "index")
889+
init_iter = self.cse.namedvar("init_iter", dtype=mlir_common.INDEX_DTYPE)
878890
parallel_map = f"affine.parallel (%{init_iter}) = ({0}) to ({compute_vec_size}) {{ // Base vector initializer"
879891
self.spad_buffer.writeline(parallel_map)
880892
with self.spad_buffer.indent():
@@ -1479,8 +1491,12 @@ def get_const_cse(self, value, dtype="index") -> common.CSEVariable:
14791491
value = int(value)
14801492
key = str(value)+dtype
14811493
if key not in self.consts:
1482-
self.consts[key] = self.const_cse.generate(self.const_buffer, f"arith.constant {value} : {dtype}")
1483-
self.register_var_info(self.consts[key], [1, dtype])
1494+
# MLIR_TO_DTYPE maps "index" -> INDEX_DTYPE sentinel (not
1495+
# torch.int64, which would make mlir_dtype derive to "i64").
1496+
self.consts[key] = self.const_cse.generate(
1497+
self.const_buffer, f"arith.constant {value} : {dtype}",
1498+
dtype=mlir_common.MLIR_TO_DTYPE.get(dtype),
1499+
)
14841500
return self.consts[key]
14851501

14861502
def get_tag_cse(self, value=None, shape="memref<1xi32>"):
@@ -1531,15 +1547,17 @@ def convert_indirect_indexing(self, index :sympy.Expr):
15311547
if target_dim in self.spad_buffer_dict:
15321548
sram_var, _, tile_numel_per_lane, sram_index_var, tile_shape, vshape = self.spad_buffer_dict[target_dim]
15331549
else:
1534-
# FIXME.
1535-
var_info = [v for k, v in self.var_info.items() if str(k) == target_dim][0]
1536-
dtype = mlir_common.MLIR_TO_DTYPE[var_info[1]]
1550+
# Issue #238: read torch dtype directly from the csevar's attribute
1551+
# rather than round-tripping the MLIR string through MLIR_TO_DTYPE
1552+
# (which silently downcasts bool/uint8 to int8).
1553+
csevar = self.cse.varname_map[target_dim]
1554+
dtype = csevar.dtype
15371555

15381556
local_tile_desc = self.kernel_group.tile_desc
15391557
tile_numel_per_lane = local_tile_desc.get_numel_per_lane()
1540-
tile_shape = local_tile_desc.get_mlir_shape(var_info[1])
1558+
tile_shape = local_tile_desc.get_mlir_shape(csevar.mlir_dtype)
15411559
tile_vec = local_tile_desc.get_compute_vec_size()
1542-
vshape = f"vector<{var_info[0]}x{var_info[1]}>"
1560+
vshape = f"vector<{csevar.vec_size}x{csevar.mlir_dtype}>"
15431561
sram_var, sram_index_var = self.get_scratchpad_buffer(dtype, target_dim, local_tile_desc, target_dim)
15441562
self.spad_buffer_dict[target_dim] = [sram_var, local_tile_desc.get_tile_size(), tile_numel_per_lane, sram_index_var, tile_shape, vshape]
15451563

@@ -1559,7 +1577,7 @@ def convert_indirect_indexing(self, index :sympy.Expr):
15591577
if "tmp" not in str(arg):
15601578
continue
15611579
if arg.is_Mul and arg.args[0].is_number:
1562-
coeff_dtype = self.var_info[spad_vars[str(arg.args[1])]][1]
1580+
coeff_dtype = spad_vars[str(arg.args[1])].mlir_dtype
15631581
coeff = self.get_const_cse(int(arg.args[0]), coeff_dtype)
15641582
spad_vars[str(arg.args[1])] = ops.mul(spad_vars[str(arg.args[1])], coeff)
15651583
index = index.replace(arg, 0)
@@ -1577,7 +1595,7 @@ def convert_indirect_indexing(self, index :sympy.Expr):
15771595
ops._store(spad_vars[first_dim], sram_var, sram_index_var, tile_shape) # FIXME. Maybe require fine grain compute...
15781596

15791597
# Conversion
1580-
mlir_dtype = self.var_info[spad_vars[first_dim]][1]
1598+
mlir_dtype = spad_vars[first_dim].mlir_dtype
15811599
with self.override_buffer_cse(buffer=target_dma_buffers):
15821600
out = ops._load(1, mlir_dtype, sram_var, sram_index_var, tile_shape)
15831601
if mlir_dtype != "index":

0 commit comments

Comments
 (0)