From 8c1865c5a989e48b6c3ccbf52e66a11187d57abf Mon Sep 17 00:00:00 2001 From: John Demme Date: Mon, 7 Jul 2025 12:43:12 -0700 Subject: [PATCH] [ESI] Make RAM delarations' addresses unsigned (#8658) Using signless ints as the address was just wrong. It was probably done to avoid incomplete lowerings, which have been addressed. Changes RAM declaration addresses to use unsigned integer types throughout the ESI dialect and its tests, updating both the code generator and expected outputs. - Switch address ports to unsigned (uiN) in MLIR tests and service definitions. - Update service port generation and SystemVerilog memory instantiation to use unsigned address types, inserting bitcasts to signless for indexing. - Adjust PyCDE front end and software integration tests to use UInt addressing. --- .../integration_test/test_software/esi_ram.py | 6 ++--- frontends/PyCDE/src/pycde/esi.py | 2 +- frontends/PyCDE/test/test_esi.py | 8 +++--- .../Dialect/ESI/runtime/loopback.mlir | 2 +- lib/Dialect/ESI/ESIServices.cpp | 12 ++++++--- lib/Dialect/ESI/ESIStdServices.cpp | 8 ++++-- test/Dialect/ESI/services.mlir | 26 ++++++++++--------- test/Dialect/Handshake/lower-extmem-esi.mlir | 8 +++--- 8 files changed, 42 insertions(+), 30 deletions(-) diff --git a/frontends/PyCDE/integration_test/test_software/esi_ram.py b/frontends/PyCDE/integration_test/test_software/esi_ram.py index c9f61b54b5..2c4119705e 100644 --- a/frontends/PyCDE/integration_test/test_software/esi_ram.py +++ b/frontends/PyCDE/integration_test/test_software/esi_ram.py @@ -36,7 +36,7 @@ assert dummy_info is not None def read(addr: int) -> bytearray: - mem_read_addr.write([addr]) + mem_read_addr.write(addr) resp = cast(bytearray, mem_read_data.read()) print(f"resp: {resp}") return resp @@ -45,7 +45,7 @@ def read(addr: int) -> bytearray: # The contents of address 3 are continuously updated to the contents of address # 2 by the accelerator. data = bytearray([random.randint(0, 2**8 - 1) for _ in range(8)]) -mem_write.write({"address": [2], "data": data}) +mem_write.write({"address": 2, "data": data}) resp = read(2) try_count = 0 @@ -64,6 +64,6 @@ assert resp == data # Check this by writing to address 3 and reading from it. Shouldn't have # changed. zeros = bytearray([0] * 8) -mem_write.write({"address": [3], "data": zeros}) +mem_write.write({"address": 3, "data": zeros}) resp = read(3) assert resp == data diff --git a/frontends/PyCDE/src/pycde/esi.py b/frontends/PyCDE/src/pycde/esi.py index ed6ee6df3d..7b297fe554 100644 --- a/frontends/PyCDE/src/pycde/esi.py +++ b/frontends/PyCDE/src/pycde/esi.py @@ -465,7 +465,7 @@ def DeclareRandomAccessMemory(inner_type: Type, class DeclareRandomAccessMemory(ServiceDecl): __name__ = name address_width = (depth - 1).bit_length() - address_type = Bits(address_width) + address_type = UInt(address_width) write_struct = StructType([('address', address_type), ('data', inner_type)]) read = Bundle([ diff --git a/frontends/PyCDE/test/test_esi.py b/frontends/PyCDE/test/test_esi.py index 16244b758d..b8821059f6 100644 --- a/frontends/PyCDE/test/test_esi.py +++ b/frontends/PyCDE/test/test_esi.py @@ -366,11 +366,11 @@ def Writer(type): # CHECK: hw.module @Ram1(in %clk : !seq.clock, in %rst : i1) # CHECK: esi.service.instance #esi.appid<"ram"> svc @ram impl as "sv"(%clk, %rst) : (!seq.clock, i1) -> () -# CHECK: [[WR:%.+]] = esi.service.req <@ram::@write>(#esi.appid<"ram_writer"[0]>) : !esi.bundle<[!esi.channel> from "req", !esi.channel to "ack"]> +# CHECK: [[WR:%.+]] = esi.service.req <@ram::@write>(#esi.appid<"ram_writer"[0]>) : !esi.bundle<[!esi.channel> from "req", !esi.channel to "ack"]> # CHECK: %rawOutput, %valid = esi.unwrap.vr %req, %ready : !hw.struct -# CHECK: [[CASTED:%.+]] = hw.bitcast %rawOutput : (!hw.struct) -> !hw.struct -# CHECK: %chanOutput, %ready = esi.wrap.vr [[CASTED]], %valid : !hw.struct -# CHECK: %ack = esi.bundle.unpack %chanOutput from [[WR]] : !esi.bundle<[!esi.channel> from "req", !esi.channel to "ack"]> +# CHECK: [[CASTED:%.+]] = hw.bitcast %rawOutput : (!hw.struct) -> !hw.struct +# CHECK: %chanOutput, %ready = esi.wrap.vr [[CASTED]], %valid : !hw.struct +# CHECK: %ack = esi.bundle.unpack %chanOutput from [[WR]] : !esi.bundle<[!esi.channel> from "req", !esi.channel to "ack"]> # CHECK: %bundle, %req = esi.bundle.pack %ack : !esi.bundle<[!esi.channel> from "req", !esi.channel to "ack"]> # CHECK: hw.instance "Writer" sym @Writer @Writer(clk: %clk: !seq.clock, rst: %rst: i1, cmd: %bundle: !esi.bundle<[!esi.channel> from "req", !esi.channel to "ack"]>) -> () diff --git a/integration_test/Dialect/ESI/runtime/loopback.mlir b/integration_test/Dialect/ESI/runtime/loopback.mlir index 13434549e9..9c47067201 100644 --- a/integration_test/Dialect/ESI/runtime/loopback.mlir +++ b/integration_test/Dialect/ESI/runtime/loopback.mlir @@ -93,7 +93,7 @@ hw.module @LoopbackArray() { } esi.mem.ram @MemA i64 x 20 -!write = !hw.struct +!write = !hw.struct !writeBundle = !esi.bundle<[!esi.channel from "req", !esi.channel to "ack"]> hw.module @MemoryAccess1(in %clk : !seq.clock, in %rst : i1) { diff --git a/lib/Dialect/ESI/ESIServices.cpp b/lib/Dialect/ESI/ESIServices.cpp index 297fcad655..1f03c93630 100644 --- a/lib/Dialect/ESI/ESIServices.cpp +++ b/lib/Dialect/ESI/ESIServices.cpp @@ -271,8 +271,11 @@ instantiateSystemVerilogMemory(ServiceImplementReqOp implReq, // Unwrap the requested address and read from that memory location. auto addressUnwrap = b.create(toServer, dataChannel.getReady()); - Value memLoc = - b.create(mem, addressUnwrap.getRawOutput()); + Value unsignedAddress = addressUnwrap.getRawOutput(); + Value signlessAddress = b.create( + b.getIntegerType(llvm::Log2_64_Ceil(ramDecl.getDepth())), + unsignedAddress); + Value memLoc = b.create(mem, signlessAddress); auto readData = b.create(memLoc); // Set the data on the response. @@ -294,7 +297,10 @@ instantiateSystemVerilogMemory(ServiceImplementReqOp implReq, Value a = address, d = data; // So the lambda can capture. // If we're told to go, do the write. b.create(go, [&] { - Value memLoc = b.create(mem, a); + Value signlessAddress = b.create( + b.getIntegerType(llvm::Log2_64_Ceil(ramDecl.getDepth())), a); + Value memLoc = + b.create(mem, signlessAddress); b.create(memLoc, d); }); } diff --git a/lib/Dialect/ESI/ESIStdServices.cpp b/lib/Dialect/ESI/ESIStdServices.cpp index 064a7d9ddf..e68e0c6cea 100644 --- a/lib/Dialect/ESI/ESIStdServices.cpp +++ b/lib/Dialect/ESI/ESIStdServices.cpp @@ -39,7 +39,9 @@ static ServicePortInfo createReqResp(StringAttr sym, Twine name, ServicePortInfo RandomAccessMemoryDeclOp::writePortInfo() { auto *ctxt = getContext(); - auto addressType = IntegerType::get(ctxt, llvm::Log2_64_Ceil(getDepth())); + auto addressType = + IntegerType::get(ctxt, llvm::Log2_64_Ceil(getDepth()), + IntegerType::SignednessSemantics::Unsigned); // Write port hw::StructType writeType = hw::StructType::get( @@ -53,7 +55,9 @@ ServicePortInfo RandomAccessMemoryDeclOp::writePortInfo() { ServicePortInfo RandomAccessMemoryDeclOp::readPortInfo() { auto *ctxt = getContext(); - auto addressType = IntegerType::get(ctxt, llvm::Log2_64_Ceil(getDepth())); + auto addressType = + IntegerType::get(ctxt, llvm::Log2_64_Ceil(getDepth()), + IntegerType::SignednessSemantics::Unsigned); return createReqResp(getSymNameAttr(), "read", "address", addressType, "data", getInnerType()); diff --git a/test/Dialect/ESI/services.mlir b/test/Dialect/ESI/services.mlir index 62cdc873a4..298a5e0cff 100644 --- a/test/Dialect/ESI/services.mlir +++ b/test/Dialect/ESI/services.mlir @@ -121,22 +121,24 @@ esi.pure_module @LoopbackCosimPure { } // CONN-LABEL: esi.mem.ram @MemA i64 x 20 -// CONN-LABEL: hw.module @MemoryAccess1(in %clk : !seq.clock, in %rst : i1, in %write : !esi.channel>, in %readAddress : !esi.channel, out readData : !esi.channel, out writeDone : !esi.channel) { +// CONN-LABEL: hw.module @MemoryAccess1(in %clk : !seq.clock, in %rst : i1, in %write : !esi.channel>, in %readAddress : !esi.channel, out readData : !esi.channel, out writeDone : !esi.channel) { // CONN: %MemA = sv.reg : !hw.inout> // CONN: %chanOutput, %ready = esi.wrap.vr %c0_i0, %write_done : i0 -// CONN: %rawOutput, %valid = esi.unwrap.vr %write, %ready : !hw.struct -// CONN: %address = hw.struct_extract %rawOutput["address"] : !hw.struct -// CONN: %data = hw.struct_extract %rawOutput["data"] : !hw.struct +// CONN: %rawOutput, %valid = esi.unwrap.vr %write, %ready : !hw.struct +// CONN: %address = hw.struct_extract %rawOutput["address"] : !hw.struct +// CONN: %data = hw.struct_extract %rawOutput["data"] : !hw.struct // CONN: %[[ANDVR:.*]] = comb.and %valid, %ready {sv.namehint = "write_go"} : i1 // CONN: %write_done = seq.compreg sym @write_done %[[ANDVR]], %clk reset %rst, %false : i1 // CONN: %chanOutput_0, %ready_1 = esi.wrap.vr %[[MEMREAD:.*]], %valid_3 : i64 -// CONN: %rawOutput_2, %valid_3 = esi.unwrap.vr %readAddress, %ready_1 : i5 -// CONN: %[[MEMREADIO:.*]] = sv.array_index_inout %MemA[%rawOutput_2] : !hw.inout>, i5 +// CONN: %rawOutput_2, %valid_3 = esi.unwrap.vr %readAddress, %ready_1 : ui5 +// CONN: %[[ADDR:.*]] = hw.bitcast %rawOutput_2 : (ui5) -> i5 +// CONN: %[[MEMREADIO:.*]] = sv.array_index_inout %MemA[%[[ADDR]]] : !hw.inout>, i5 // CONN: %[[MEMREAD]] = sv.read_inout %[[MEMREADIO]] : !hw.inout // CONN: %[[CLOCK:.+]] = seq.from_clock %clk // CONN: sv.alwaysff(posedge %[[CLOCK]]) { // CONN: sv.if %[[ANDVR]] { -// CONN: %[[ARRIDX:.*]] = sv.array_index_inout %MemA[%address] : !hw.inout>, i5 +// CONN: %[[ADDR:.*]] = hw.bitcast %address : (ui5) -> i5 +// CONN: %[[ARRIDX:.*]] = sv.array_index_inout %MemA[%[[ADDR]]] : !hw.inout>, i5 // CONN: sv.passign %[[ARRIDX]], %data : i64 // CONN: } // CONN: }(syncreset : posedge %rst) { @@ -144,11 +146,11 @@ esi.pure_module @LoopbackCosimPure { // CONN: hw.output %chanOutput_0, %chanOutput : !esi.channel, !esi.channel esi.mem.ram @MemA i64 x 20 -!write = !hw.struct +!write = !hw.struct !writeBundle = !esi.bundle<[!esi.channel from "req", !esi.channel to "ack"]> -!readBundle = !esi.bundle<[!esi.channel from "address", !esi.channel to "data"]> +!readBundle = !esi.bundle<[!esi.channel from "address", !esi.channel to "data"]> -hw.module @MemoryAccess1(in %clk : !seq.clock, in %rst : i1, in %write : !esi.channel, in %readAddress : !esi.channel, out readData : !esi.channel, out writeDone : !esi.channel) { +hw.module @MemoryAccess1(in %clk : !seq.clock, in %rst : i1, in %write : !esi.channel, in %readAddress : !esi.channel, out readData : !esi.channel, out writeDone : !esi.channel) { esi.service.instance #esi.appid<"mem"> svc @MemA impl as "sv_mem" (%clk, %rst) : (!seq.clock, i1) -> () %writeBundle = esi.service.req <@MemA::@write> (#esi.appid<"write">) : !writeBundle %done = esi.bundle.unpack %write from %writeBundle : !writeBundle @@ -158,11 +160,11 @@ hw.module @MemoryAccess1(in %clk : !seq.clock, in %rst : i1, in %write : !esi.ch hw.output %readData, %done : !esi.channel, !esi.channel } -// CONN-LABEL: hw.module @MemoryAccess2Read(in %clk : !seq.clock, in %rst : i1, in %write : !esi.channel>, in %readAddress : !esi.channel, in %readAddress2 : !esi.channel, out readData : !esi.channel, out readData2 : !esi.channel, out writeDone : !esi.channel) { +// CONN-LABEL: hw.module @MemoryAccess2Read(in %clk : !seq.clock, in %rst : i1, in %write : !esi.channel>, in %readAddress : !esi.channel, in %readAddress2 : !esi.channel, out readData : !esi.channel, out readData2 : !esi.channel, out writeDone : !esi.channel) { // CONN: %MemA = sv.reg : !hw.inout> // CONN: hw.output %chanOutput_0, %chanOutput_4, %chanOutput : !esi.channel, !esi.channel, !esi.channel -hw.module @MemoryAccess2Read(in %clk: !seq.clock, in %rst: i1, in %write: !esi.channel, in %readAddress: !esi.channel, in %readAddress2: !esi.channel, out readData: !esi.channel, out readData2: !esi.channel, out writeDone: !esi.channel) { +hw.module @MemoryAccess2Read(in %clk: !seq.clock, in %rst: i1, in %write: !esi.channel, in %readAddress: !esi.channel, in %readAddress2: !esi.channel, out readData: !esi.channel, out readData2: !esi.channel, out writeDone: !esi.channel) { esi.service.instance #esi.appid<"mem"> svc @MemA impl as "sv_mem" (%clk, %rst) : (!seq.clock, i1) -> () %writeBundle = esi.service.req <@MemA::@write> (#esi.appid<"write">) : !writeBundle diff --git a/test/Dialect/Handshake/lower-extmem-esi.mlir b/test/Dialect/Handshake/lower-extmem-esi.mlir index cffaf54704..279a16ef1a 100644 --- a/test/Dialect/Handshake/lower-extmem-esi.mlir +++ b/test/Dialect/Handshake/lower-extmem-esi.mlir @@ -27,10 +27,10 @@ // CHECK-SAME: in %reset : i1, // CHECK-SAME: out out0 : !esi.channel // CHECK-SAME: ) { -//CHECK-NEXT: [[B0:%.+]] = esi.service.req <@mem::@read>(#esi.appid<"load"[1]>) : !esi.bundle<[!esi.channel from "address", !esi.channel to "data"]> -//CHECK-NEXT: %data = esi.bundle.unpack %main.mem_ld0.addr from [[B0]] : !esi.bundle<[!esi.channel from "address", !esi.channel to "data"]> -//CHECK-NEXT: [[B1:%.+]] = esi.service.req <@mem::@write>(#esi.appid<"store"[2]>) : !esi.bundle<[!esi.channel> from "req", !esi.channel to "ack"]> -//CHECK-NEXT: %ack = esi.bundle.unpack %main.mem_st0 from [[B1]] : !esi.bundle<[!esi.channel> from "req", !esi.channel to "ack"]> +//CHECK-NEXT: [[B0:%.+]] = esi.service.req <@mem::@read>(#esi.appid<"load"[1]>) : !esi.bundle<[!esi.channel from "address", !esi.channel to "data"]> +//CHECK-NEXT: %data = esi.bundle.unpack %main.mem_ld0.addr from [[B0]] : !esi.bundle<[!esi.channel from "address", !esi.channel to "data"]> +//CHECK-NEXT: [[B1:%.+]] = esi.service.req <@mem::@write>(#esi.appid<"store"[2]>) : !esi.bundle<[!esi.channel> from "req", !esi.channel to "ack"]> +//CHECK-NEXT: %ack = esi.bundle.unpack %main.mem_st0 from [[B1]] : !esi.bundle<[!esi.channel> from "req", !esi.channel to "ack"]> //CHECK-NEXT: %main.out0, %main.mem_ld0.addr, %main.mem_st0 = hw.instance "main" @__main_hw(arg0: %arg0: !esi.channel, arg1: %arg1: !esi.channel, v: %v: !esi.channel, mem_ld0.data: %data: !esi.channel, mem_st0.done: %ack: !esi.channel, argCtrl: %argCtrl: !esi.channel, clock: %clock: !seq.clock, reset: %reset: i1) -> (out0: !esi.channel, mem_ld0.addr: !esi.channel, mem_st0: !esi.channel>) //CHECK-NEXT: hw.output %main.out0 : !esi.channel //CHECK-NEXT: }