From a4fb642e7b9241c4cb1acb74f5738104d0c3bda6 Mon Sep 17 00:00:00 2001 From: rsetaluri Date: Thu, 6 Apr 2023 14:15:27 -0700 Subject: [PATCH] [MLIR] Prefix tuple field keys with _ See #1258. --- magma/backend/mlir/hardware_module.py | 26 ++++++++++++++++--- .../golds/simple_aggregates_tuple.mlir | 10 +++---- .../test_mlir/golds/simple_aggregates_tuple.v | 9 +++++++ .../test_compile_to_mlir_local_examples.py | 5 ---- 4 files changed, 37 insertions(+), 13 deletions(-) create mode 100644 tests/test_backend/test_mlir/golds/simple_aggregates_tuple.v diff --git a/magma/backend/mlir/hardware_module.py b/magma/backend/mlir/hardware_module.py index 6dd3343ef..480e36efa 100644 --- a/magma/backend/mlir/hardware_module.py +++ b/magma/backend/mlir/hardware_module.py @@ -70,7 +70,7 @@ from magma.primitives.xmr import XMRSink, XMRSource from magma.protocol_type import magma_value as get_magma_value from magma.t import Kind, Type -from magma.tuple import TupleMeta, Tuple as m_Tuple +from magma.tuple import TupleMeta, Tuple as m_Tuple, Product from magma.value_utils import make_selector, TupleSelector, ArraySelector from magma.view import PortView @@ -138,9 +138,21 @@ def magma_type_to_mlir_type(type: Kind) -> MlirType: if issubclass(type.T, Bit): return magma_type_to_mlir_type(Bits[type.N]) return hw.ArrayType((type.N,), magma_type_to_mlir_type(type.T)) + # NOTE(rsetaluri): If the type is Tuple (i.e. has integer keys), then the + # struct type we emit should prefix the field key with an underscore + # (e.g. "_0") to make it a valid keyword. Note that this *must* be in sync + # with the translation of magma_tuple_get_op's which does the same. + if issubclass(type, Product): + fields = { + str(k): magma_type_to_mlir_type(t) + for k, t in type.field_dict.items() + } + return hw.StructType(tuple(fields.items())) if issubclass(type, m_Tuple): - fields = {str(k): magma_type_to_mlir_type(t) - for k, t in type.field_dict.items()} + fields = { + f"_{i}": magma_type_to_mlir_type(t) + for i, t in enumerate(type.field_dict.values()) + } return hw.StructType(tuple(fields.items())) @@ -970,6 +982,14 @@ def visit_instance_wrapper(self, module: ModuleWrapper) -> bool: return True if inst_wrapper.name.startswith("magma_tuple_get_op"): index = inst_wrapper.attrs["index"] + # NOTE(rsetaluri): If the underlying type is *not* a Product (i.e. a + # Tuple with integer keys), then the StructExtractOp we emit should + # prefix the field key with an underscore (e.g. "_0") to make it a + # valid keyword. Note that this *must* be in sync with the + # translation function magma_type_to_mlir_type() which does the + # same. + if not issubclass(inst_wrapper.attrs["T"], Product): + index = f"_{index}" hw.StructExtractOp( field=index, operands=module.operands, diff --git a/tests/test_backend/test_mlir/golds/simple_aggregates_tuple.mlir b/tests/test_backend/test_mlir/golds/simple_aggregates_tuple.mlir index 1545b1e75..5fee293e5 100644 --- a/tests/test_backend/test_mlir/golds/simple_aggregates_tuple.mlir +++ b/tests/test_backend/test_mlir/golds/simple_aggregates_tuple.mlir @@ -1,11 +1,11 @@ module attributes {circt.loweringOptions = "locationInfoStyle=none"} { - hw.module @simple_aggregates_tuple(%a: !hw.struct<0: i8, 1: i8>) -> (y: !hw.struct<0: i8, 1: i8>) { - %0 = hw.struct_extract %a["0"] : !hw.struct<0: i8, 1: i8> + hw.module @simple_aggregates_tuple(%a: !hw.struct<_0: i8, _1: i8>) -> (y: !hw.struct<_0: i8, _1: i8>) { + %0 = hw.struct_extract %a["_0"] : !hw.struct<_0: i8, _1: i8> %2 = hw.constant -1 : i8 %1 = comb.xor %2, %0 : i8 - %3 = hw.struct_extract %a["1"] : !hw.struct<0: i8, 1: i8> + %3 = hw.struct_extract %a["_1"] : !hw.struct<_0: i8, _1: i8> %4 = comb.xor %2, %3 : i8 - %5 = hw.struct_create (%1, %4) : !hw.struct<0: i8, 1: i8> - hw.output %5 : !hw.struct<0: i8, 1: i8> + %5 = hw.struct_create (%1, %4) : !hw.struct<_0: i8, _1: i8> + hw.output %5 : !hw.struct<_0: i8, _1: i8> } } diff --git a/tests/test_backend/test_mlir/golds/simple_aggregates_tuple.v b/tests/test_backend/test_mlir/golds/simple_aggregates_tuple.v new file mode 100644 index 000000000..c2a1e3cb7 --- /dev/null +++ b/tests/test_backend/test_mlir/golds/simple_aggregates_tuple.v @@ -0,0 +1,9 @@ +// Generated by CIRCT circtorg-0.0.0-1773-g7abbc4313 +module simple_aggregates_tuple( + input struct packed {logic [7:0] _0; logic [7:0] _1; } a, + output struct packed {logic [7:0] _0; logic [7:0] _1; } y +); + + assign y = '{_0: (~a._0), _1: (~a._1)}; +endmodule + diff --git a/tests/test_backend/test_mlir/test_compile_to_mlir_local_examples.py b/tests/test_backend/test_mlir/test_compile_to_mlir_local_examples.py index 7d0a3aacd..cc12f7653 100644 --- a/tests/test_backend/test_mlir/test_compile_to_mlir_local_examples.py +++ b/tests/test_backend/test_mlir/test_compile_to_mlir_local_examples.py @@ -43,11 +43,6 @@ def test_compile_to_mlir(ckt): "use_native_bind_processor": True, "basename": ckt.name, } - # NOTE(rsetaluri): This is a hack to skip the Tuple[] to verilog test since - # MLIR does not allow leading integers in names (e.g. `!hw.struct<0: - # ...>`). We should ultimately fix this by changing the IR code generation. - if ckt.name == "simple_aggregates_tuple": - kwargs["check_verilog"] = False run_test_compile_to_mlir(ckt, **kwargs)