From 66e6ff76edcf8bd8207fbb909c1fbbe583ae27b4 Mon Sep 17 00:00:00 2001 From: Chen Xi <48302201+Ivyfeather@users.noreply.github.com> Date: Wed, 25 Sep 2024 18:08:27 +0800 Subject: [PATCH] fix(DataStorage): Fix req_valid and Add MCP2 Check Assertions (#254) * TMP: add HoldChecker to ensure MCP2 Hold condition * misc: an official version of MCP2 hold check * misc: add check right at predecessor regs' output * misc: remove checks that are no longer needed * DataStorage: use LSB instead of orR for req_valid Since task_s3_valid_hold2(1) will change during the two cycles, we just use (0) as req_valid * misc: add check for task_valid_s3_hold2(0) * misc: fix wrong assertion --- src/main/scala/coupledL2/CoupledL2.scala | 1 + src/main/scala/coupledL2/Directory.scala | 17 ++++++ src/main/scala/coupledL2/L2Param.scala | 2 + src/main/scala/coupledL2/MSHRBuffer.scala | 6 ++ src/main/scala/coupledL2/SinkC.scala | 7 +++ .../scala/coupledL2/tl2chi/MainPipe.scala | 8 ++- src/main/scala/coupledL2/tl2chi/Slice.scala | 12 ++++ src/main/scala/coupledL2/tl2tl/MainPipe.scala | 7 ++- src/main/scala/coupledL2/tl2tl/Slice.scala | 11 ++++ .../scala/coupledL2/utils/HoldChecker.scala | 59 +++++++++++++++++++ 10 files changed, 128 insertions(+), 2 deletions(-) create mode 100644 src/main/scala/coupledL2/utils/HoldChecker.scala diff --git a/src/main/scala/coupledL2/CoupledL2.scala b/src/main/scala/coupledL2/CoupledL2.scala index 2bac7874..19a68b7a 100644 --- a/src/main/scala/coupledL2/CoupledL2.scala +++ b/src/main/scala/coupledL2/CoupledL2.scala @@ -79,6 +79,7 @@ trait HasCoupledL2Parameters { def hasPrefetchSrc = prefetchers.exists(_.hasPrefetchSrc) def hasCMO = cacheParams.hasCMO def topDownOpt = if(cacheParams.elaboratedTopDown) Some(true) else None + def hasMCP2Check = cacheParams.MCP2Check def enableHintGuidedGrant = true diff --git a/src/main/scala/coupledL2/Directory.scala b/src/main/scala/coupledL2/Directory.scala index 377896ec..37743247 100644 --- a/src/main/scala/coupledL2/Directory.scala +++ b/src/main/scala/coupledL2/Directory.scala @@ -113,6 +113,7 @@ class Directory(implicit p: Parameters) extends L2Module { val replResp = ValidIO(new ReplacerResult) // used to count occWays for Grant to retry val msInfo = Vec(mshrsAll, Flipped(ValidIO(new MSHRInfo))) + val mcp2Check = if(hasMCP2Check) Some(Input(new MCP2CheckEn)) else None }) def invalid_way_sel(metaVec: Seq[MetaEntry], repl: UInt) = { @@ -373,4 +374,20 @@ class Directory(implicit p: Parameters) extends L2Module { XSPerfAccumulate("dirRead_cnt", io.read.fire) XSPerfAccumulate("choose_busy_way", reqValid_s3 && !req_s3.wayMask(chosenWay)) + + // ===== for MCP2 hold check ===== + if (hasMCP2Check) { + // TODO: it now relies on physical design analysis to tell us which regs to check hold + // We may use chisel methods to distinguish all predecessor registers + // of dataStorage inputs in the future + val en = io.mcp2Check.get.en + HoldChecker.check2(io.resp.bits, en, "dirResp_s3") + HoldChecker.check2(io.replResp.bits, en, "replResp_s3") + + HoldChecker.check2(freeWayMask_s3, en, "freeWayMask_s3") + HoldChecker.check2(metaAll_s3, en, "metaAll_s3") + HoldChecker.check2(repl_state_s3, en, "repl_state_s3") + HoldChecker.check2(req_s3, en, "req_s3") + HoldChecker.check2(tagAll_s3, en, "tagAll_s3") + } } diff --git a/src/main/scala/coupledL2/L2Param.scala b/src/main/scala/coupledL2/L2Param.scala index 13f972eb..ef93607e 100644 --- a/src/main/scala/coupledL2/L2Param.scala +++ b/src/main/scala/coupledL2/L2Param.scala @@ -106,6 +106,8 @@ case class L2Param( FPGAPlatform: Boolean = false, // CMO hasCMO: Boolean = false, + // has DataStorage MCP2 Check + MCP2Check: Boolean = true, // Network layer SAM sam: Seq[(AddressSet, Int)] = Seq(AddressSet.everything -> 0) diff --git a/src/main/scala/coupledL2/MSHRBuffer.scala b/src/main/scala/coupledL2/MSHRBuffer.scala index b181935d..6be0dce9 100644 --- a/src/main/scala/coupledL2/MSHRBuffer.scala +++ b/src/main/scala/coupledL2/MSHRBuffer.scala @@ -42,6 +42,7 @@ class MSHRBuffer(wPorts: Int = 1)(implicit p: Parameters) extends L2Module { val r = Flipped(ValidIO(new MSHRBufRead)) val resp = new MSHRBufResp val w = Vec(wPorts, Flipped(ValidIO(new MSHRBufWrite))) + val mcp2Check = if(hasMCP2Check) Some(Input(new MCP2CheckEn)) else None }) val buffer = Reg(Vec(mshrsAll, Vec(beatSize, UInt((beatBytes * 8).W)))) @@ -63,6 +64,11 @@ class MSHRBuffer(wPorts: Int = 1)(implicit p: Parameters) extends L2Module { val rdata = buffer(io.r.bits.id).asUInt io.resp.data.data := RegEnable(rdata, 0.U.asTypeOf(rdata), io.r.valid) + + if (hasMCP2Check) { + assert(!io.r.valid || !RegNext(io.r.valid), "No continuous read") + HoldChecker.check2(io.resp.data.data, io.mcp2Check.get.wen, "mshrBuf_wdata") + } } // may consider just choose an empty entry to insert diff --git a/src/main/scala/coupledL2/SinkC.scala b/src/main/scala/coupledL2/SinkC.scala index 2aecc4de..6a88265d 100644 --- a/src/main/scala/coupledL2/SinkC.scala +++ b/src/main/scala/coupledL2/SinkC.scala @@ -23,6 +23,7 @@ import freechips.rocketchip.tilelink._ import freechips.rocketchip.tilelink.TLMessages._ import org.chipsalliance.cde.config.Parameters import utility.{MemReqSource, XSPerfAccumulate, RRArbiterInit} +import coupledL2.utils._ class PipeBufferResp(implicit p: Parameters) extends L2Bundle { val data = Vec(beatSize, UInt((beatBytes * 8).W)) @@ -41,6 +42,7 @@ class SinkC(implicit p: Parameters) extends L2Module { val bufResp = Output(new PipeBufferResp) val refillBufWrite = ValidIO(new MSHRBufWrite) val msInfo = Vec(mshrsAll, Flipped(ValidIO(new MSHRInfo))) + val mcp2Check = if(hasMCP2Check) Some(Input(new MCP2CheckEn)) else None }) val (first, last, _, beat) = edgeIn.count(io.c) @@ -196,4 +198,9 @@ class SinkC(implicit p: Parameters) extends L2Module { XSPerfAccumulate("NewDataNestC", io.refillBufWrite.valid) //!!WARNING: TODO: if this is zero, that means fucntion [Release-new-data written into refillBuf] // is never tested, and may have flaws + // ===== for MCP2 hold check ===== + if (hasMCP2Check) { + assert(!io.task.fire || !RegNext(io.task.fire), "No continuous write @SinkC") + HoldChecker.check2(io.bufResp.data, io.mcp2Check.get.wen, "sinkC_wdata") + } } diff --git a/src/main/scala/coupledL2/tl2chi/MainPipe.scala b/src/main/scala/coupledL2/tl2chi/MainPipe.scala index 22a1bf46..f5f7139f 100644 --- a/src/main/scala/coupledL2/tl2chi/MainPipe.scala +++ b/src/main/scala/coupledL2/tl2chi/MainPipe.scala @@ -25,6 +25,7 @@ import freechips.rocketchip.tilelink.TLMessages._ import freechips.rocketchip.tilelink.TLPermissions._ import org.chipsalliance.cde.config.Parameters import coupledL2._ +import coupledL2.utils.HoldChecker import coupledL2.prefetch.{PrefetchTrain, PfSource} import coupledL2.tl2chi.CHICohStates._ import coupledL2.MetaData._ @@ -396,7 +397,7 @@ class MainPipe(implicit p: Parameters) extends TL2CHIL2Module with HasCHIOpcodes } io.toDS.en_s3 := task_s3.valid && (ren || wen) - io.toDS.req_s3.valid := task_s3_valid_hold2.orR && (ren || wen) + io.toDS.req_s3.valid := task_s3_valid_hold2(0) && (ren || wen) io.toDS.req_s3.bits.way := Mux( mshr_refill_s3 && req_s3.replTask, io.replResp.bits.way, @@ -414,6 +415,11 @@ class MainPipe(implicit p: Parameters) extends TL2CHIL2Module with HasCHIOpcodes ) ) + if (hasMCP2Check) { + HoldChecker.check2(task_s3.bits, io.toDS.en_s3, "task_s3_bits") + HoldChecker.check2(task_s3_valid_hold2(0), io.toDS.en_s3, "task_s3_valid_hold2_0") + } + /* ======== Read DS and store data in Buffer ======== */ // A: need_write_releaseBuf indicates that DS should be read and the data will be written into ReleaseBuffer // need_write_releaseBuf is assigned true when: diff --git a/src/main/scala/coupledL2/tl2chi/Slice.scala b/src/main/scala/coupledL2/tl2chi/Slice.scala index 7e4c1954..c2b45068 100644 --- a/src/main/scala/coupledL2/tl2chi/Slice.scala +++ b/src/main/scala/coupledL2/tl2chi/Slice.scala @@ -22,6 +22,7 @@ import chisel3.util._ import freechips.rocketchip.tilelink._ import org.chipsalliance.cde.config.Parameters import coupledL2._ +import coupledL2.utils._ import coupledL2.prefetch.PrefetchIO class OuterBundle(implicit p: Parameters) extends DecoupledPortIO with BaseOuterBundle @@ -209,4 +210,15 @@ class Slice()(implicit p: Parameters) extends BaseSlice[OuterBundle] rxrsp.io.out <> io.out.rx.rsp io_pCrd <> mshrCtl.io.pCrd + + if (hasMCP2Check) { + val dsEnable = WireInit(0.U.asTypeOf(new MCP2CheckEn)) + dsEnable.en := dataStorage.io.en + dsEnable.wen := dataStorage.io.en && dataStorage.io.req.bits.wen + + directory.io.mcp2Check.get := dsEnable + releaseBuf.io.mcp2Check.get := dsEnable + refillBuf.io.mcp2Check.get := dsEnable + sinkC.io.mcp2Check.get := dsEnable + } } diff --git a/src/main/scala/coupledL2/tl2tl/MainPipe.scala b/src/main/scala/coupledL2/tl2tl/MainPipe.scala index 35ba0e16..64782922 100644 --- a/src/main/scala/coupledL2/tl2tl/MainPipe.scala +++ b/src/main/scala/coupledL2/tl2tl/MainPipe.scala @@ -299,7 +299,7 @@ class MainPipe(implicit p: Parameters) extends L2Module { } io.toDS.en_s3 := task_s3.valid && (ren || wen) - io.toDS.req_s3.valid := task_s3_valid_hold2.orR && (ren || wen) + io.toDS.req_s3.valid := task_s3_valid_hold2(0) && (ren || wen) io.toDS.req_s3.bits.way := Mux(mshr_refill_s3 && req_s3.replTask, io.replResp.bits.way, Mux(mshr_req_s3, req_s3.way, dirResult_s3.way)) io.toDS.req_s3.bits.set := Mux(mshr_req_s3, req_s3.set, dirResult_s3.set) @@ -314,6 +314,11 @@ class MainPipe(implicit p: Parameters) extends L2Module { ) ) + if (hasMCP2Check) { + HoldChecker.check2(task_s3.bits, io.toDS.en_s3, "task_s3_bits") + HoldChecker.check2(task_s3_valid_hold2(0), io.toDS.en_s3, "task_s3_valid_hold2_0") + } + /* ======== Read DS and store data in Buffer ======== */ // A: need_write_releaseBuf indicates that DS should be read and the data will be written into ReleaseBuffer // need_write_releaseBuf is assigned true when: diff --git a/src/main/scala/coupledL2/tl2tl/Slice.scala b/src/main/scala/coupledL2/tl2tl/Slice.scala index edaad502..52f4892a 100644 --- a/src/main/scala/coupledL2/tl2tl/Slice.scala +++ b/src/main/scala/coupledL2/tl2tl/Slice.scala @@ -205,4 +205,15 @@ class Slice()(implicit p: Parameters) extends BaseSlice[OuterBundle] { val monitor = Module(new Monitor()) monitor.io.fromMainPipe <> mainPipe.io.toMonitor + + if (hasMCP2Check) { + val dsEnable = WireInit(0.U.asTypeOf(new MCP2CheckEn)) + dsEnable.en := dataStorage.io.en + dsEnable.wen := dataStorage.io.en && dataStorage.io.req.bits.wen + + directory.io.mcp2Check.get := dsEnable + releaseBuf.io.mcp2Check.get := dsEnable + refillBuf.io.mcp2Check.get := dsEnable + sinkC.io.mcp2Check.get := dsEnable + } } diff --git a/src/main/scala/coupledL2/utils/HoldChecker.scala b/src/main/scala/coupledL2/utils/HoldChecker.scala new file mode 100644 index 00000000..5cc4de11 --- /dev/null +++ b/src/main/scala/coupledL2/utils/HoldChecker.scala @@ -0,0 +1,59 @@ +/** ************************************************************************************* + * Copyright (c) 2020-2021 Institute of Computing Technology, Chinese Academy of Sciences + * Copyright (c) 2020-2021 Peng Cheng Laboratory + * + * XiangShan is licensed under Mulan PSL v2. + * You can use this software according to the terms and conditions of the Mulan PSL v2. + * You may obtain a copy of Mulan PSL v2 at: + * http://license.coscl.org.cn/MulanPSL2 + * + * THIS SOFTWARE IS PROVIDED ON AN "AS IS" BASIS, WITHOUT WARRANTIES OF ANY KIND, + * EITHER EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO NON-INFRINGEMENT, + * MERCHANTABILITY OR FIT FOR A PARTICULAR PURPOSE. + * + * See the Mulan PSL v2 for more details. + * ************************************************************************************* + */ + +package coupledL2.utils + +import chisel3._ +import chisel3.util._ + +// enable signals, only used to check mcp2 hold condition of predecessor regs +class MCP2CheckEn extends Bundle { + val en = Bool() + val wen = Bool() +} + +/** + * Assert the signal must hold for certain cycles when enable is high + */ +object HoldChecker { + /** + * signal holds at en and the next cycle + */ + def check2(signal: Data, en: Bool, name: String): Unit = { + // at the 2nd cycle, signal changes + assert(!(RegNext(en) && (signal.asUInt =/= RegNext(signal).asUInt)), + s"signal changed at $name, fails to hold for 2 cycles") + } + + /** + * signal holds for N cycles + */ + def apply(signal: Data, en: Bool, cycles: Int, name: String): Unit = { + val counter = RegInit(0.U(log2Ceil(cycles).W)) + val data = RegEnable(signal, 0.U.asTypeOf(signal), en) + + when(en) { + counter := cycles.U - 1.U + }.otherwise { + counter := Mux(counter === 0.U, 0.U, counter - 1.U) + } + + assert((counter =/= 0.U) && (signal.asUInt =/= data.asUInt), + s"Signal should hold for $cycles cycles, but it fails") + } +} +