Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize truth table merge (backport #3993) #3995

Merged
merged 3 commits into from
Apr 12, 2024
Merged

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Apr 12, 2024

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:

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.

This is an automatic backport of pull request #3993 done by [Mergify](https://mergify.com).

This allows it to test package private methods.

(cherry picked from commit 09d6fa7)
Most TruthTables have a default of all dont care. We can optimize this
common case by doing actual work in TruthTable.split and
TruthTable.merge.

(cherry picked from commit bb32df4)
@mergify mergify bot added the Backport Automated backport, please consider for minor release label Apr 12, 2024
@github-actions github-actions bot added Merge with merge commit Please merge with a merge commit, do not squash and merg Performance Improves performance, will be included in release notes labels Apr 12, 2024
@chiselbot chiselbot merged commit 91edc11 into 6.x Apr 12, 2024
17 checks passed
@chiselbot chiselbot deleted the mergify/bp/6.x/pr-3993 branch April 12, 2024 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport Automated backport, please consider for minor release Merge with merge commit Please merge with a merge commit, do not squash and merg Performance Improves performance, will be included in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants