ucb-bar / constellation

A Chisel RTL generator for network-on-chip interconnects
http://constellation.readthedocs.io
BSD 3-Clause "New" or "Revised" License
177 stars 25 forks source link

`DiplomaticNetworkNodeMapping` fails with more than 9 cores #54

Open mayyxeng opened 1 year ago

mayyxeng commented 1 year ago

Method getNode in DiplomaticNetworkNodeMapping cannot handle more than 9 cores. The issue is with how string matching is performed, consider:

getNode("Core 10 DCache[0],Core 10 DCache MMIO[0],Core 10 ICache[0]", ListMap("Core 10" -> 1, "Core 1" -> 0))

Here we will have matches = List(true, true), because "Core 10" contains both "Core 1" and "Core 10". I would say the desired results should have been matches = List(true, false).

A quick fix is:

 def getNode(l: String, nodeMapping: ListMap[String, Int]): Option[Int] = {
    val keys = nodeMapping.keys.toSeq
    val patterns = keys.map { k =>
      val regexSpecial = Seq(
        "+","*", "?", "^", "$", "(", ")", "[", "]", "{", "}","|"
      )
      val escaped = regexSpecial.foldLeft(k) { case (kk, ch) =>
        kk.replace(ch, "\\" + ch)
      }
      raw".*${escaped}[A-Z,a-z,\,,\[,\],\s,\|]+.*".r
    }
    val matches = patterns.map { p => p.findFirstIn(l) }
    if (matches.filter(_.nonEmpty).size == 1) {
      val index = matches.indexWhere(_.nonEmpty)
      Some(nodeMapping.values.toSeq(index))
    } else {
      None
    }
  }
jerryz123 commented 1 year ago

Thanks for reporting and posting a fix.

I don't remember precisely, but does "Core 1 " (note the space) resolve this too? It's quite the hack, but I seem to recall that's how I got around this issue.

Obviously the more fundamental problem is using this string matching system in the first place... I couldn't find a better way to do this without major API additions elsewhere.

mayyxeng commented 1 year ago

Thanks for the quick response.

Yes "Core 1 " (with white-space) does solve the issue. And that is a much simpler and better hack than regular expression.