Optimize truth table merge

[jackkoenig]

I'm collecting numbers, but it should be pretty obvious that not essentially doing a deep copy of every TruthTable both before and after running espresso minimization in the common case where the default value is uniform (i.e. all ones, all zeros, or, most commonly, all dont care) will speed things up.

To implement this, I also added some useful methods to BitPat that you can see the tests for.

Doing split and merge on the TruthTable from TruthTableSpec:

  val table = TruthTable(
    Map(
      // BitPat("b000") -> BitPat("b0"),
      BitPat("b001") -> BitPat("b?"),
      BitPat("b010") -> BitPat("b?"),
      // BitPat("b011") -> BitPat("b0"),
      BitPat("b100") -> BitPat("b1"),
      BitPat("b101") -> BitPat("b1"),
      // BitPat("b110") -> BitPat("b0"),
      BitPat("b111") -> BitPat("b1")
    ),
    BitPat("b0")
  )

Before this change:

Benchmark               Mode  Cnt  Score   Error   Units
ChiselBenchmark.Merge  thrpt   10  0.366 ± 0.003  ops/us
ChiselBenchmark.Split  thrpt   10  0.786 ± 0.006  ops/us

After this change:

Benchmark               Mode  Cnt    Score   Error   Units
ChiselBenchmark.Merge  thrpt   10  453.041 ± 1.422  ops/us
ChiselBenchmark.Split  thrpt   10   83.122 ± 0.184  ops/us

Also from a large real design, I measured it was spending 2 minutes in the little filter function in split:

chisel/src/main/scala/chisel3/util/experimental/decode/TruthTable.scala

Line 145 in d584dd7

def bpFilter(bitPat: BitPat, indexes: Seq[Int]): BitPat =

(this should use a Set instead of a Seq)
With this change, it now spends 0.3ms in that function which just shows how much unnecessary splitting we were doing 🙃.

Contributor Checklist

• Did you add Scaladoc to every public function/method?
• Did you add at least one test demonstrating the PR?
• Did you delete any extraneous printlns/debugging code?
• Did you specify the type of improvement?
• Did you add appropriate documentation in docs/src?
• Did you request a desired merge strategy?
• Did you add text to be included in the Release Notes for this change?

Type of Improvement

• Performance improvement

Desired Merge Strategy

• Rebase

Release Notes

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
• Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you do one of the following when ready to merge:
  • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
  • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

[sequencer]

width of bp cannot be 0. Maybe assert out?

[jackkoenig]

I'm future proofing this such that if we do allow it, these should be correct for it (see stubs in tests).

[jackkoenig]

I have added benchmarking results, not extremely rigorous but in practice this is a huge improvement.

[sequencer]

With this change, it now spends 0.3ms in that function which just shows how much unnecessary splitting we were doing 🙃.

the reason of splitting is because cannot support the existence of both 0, 1 in the result of table. To support this, we have to split it to multiple runs of espresso.

[sequencer]

LGTM. Let’s make common case fast!
Since split is only designed for long decode output with 0 and 1