DecoupledHelper not able to exclude expressions

[mehnadnerd]

Type of issue: bug report | feature request

Impact: API modification

Development Phase: request

What is the current behavior?
Currently, a DecoupledHelper created as: val helper = DecoupledHelper(a || b, c) does not support helper.fire(a || b), a || b is not excluded. This is because the check used in DecoupledHelper is for referential equality, and since these are not the same reference they do not match as equal, and so it is included.

What is the expected behavior?
The expected behaviour would be that it is excluded. This can be done currently by manually making a temporary, val aorb = a || b; val helper = DecoupledHelper(aorb, c); helper.fire(aorb) works fine.

What is the use case for changing the behavior?
The use case would be making it easier to use DecoupledHelper with more complex expressions. It is useful to have these more complex boolean expressions when which Decoupled interfaces we are using depends on the state or other factors, but going through the effort of using a temporary defeats some of the convenience of using DecoupledHelper in the first place.

Impact of Changes
While this would modify existing APIs, it should be unlikely to break existing code, as any code which relied upon this is likely to be unintentional. Additionally, there are several places in RocketChip where a temporary seems to have been created, which could be eliminated thanks to this change.

[jackkoenig]

I agree that the use of referential equality here is dangerous in that it silently does something different than what one would expect in such cases.

// Current implementation
class DecoupledHelper(val rvs: Seq[Bool]) {
  def fire(exclude: Bool, includes: Bool*) = {
    (rvs.filter(_ ne exclude) ++ includes).reduce(_ && _)
  }
}
// Possible different implementation:
class DecoupledHelper(val rvs: Seq[Bool]) {
  def fire(exclude: Bool, includes: Bool*) = {
    val conj = (rvs ++ includes).reduce(_ && _)
    (conj && exclude) || (conj && !exclude)
  }
}

This would work for the simple case above, but there are potential downsides. The point of the excludes is to specifically exclude Bools given in the original construction, whereas my alternative implementation would allow injecting logic that synthesis tools will optimize away but will be hard for FIRRTL const prop to figure out. To be a little more concrete, while it is true that (a ∧ b) ∨ (a ∧ ¬b) is logically equivalent to a, FIRRTL probably won't be making that optimization any time soon. While I don't have any problem with "uglier" Verilog, it might be somewhat problematic for people using traditional coverage flows.

Another option is to check that the exclude is actually present in rvs, which, while still requiring intermediates for the given use case, at least won't silently do the wrong thing:

class DecoupledHelper(val rvs: Seq[Bool]) {
  def fire(exclude: Bool, includes: Bool*) = {
    require(rvs.contains(exclude), "Excluded Bool not present in DecoupledHelper! Note that DecoupledHelper uses referential equality for exclusion!")
    (rvs.filter(_ ne exclude) ++ includes).reduce(_ && _)
  }
}

[mehnadnerd]

Warning was merged, which leaves the decision on how (and if) we want to support excluding nontrivial expressions.

[azidar]

Please close this issue and submit an RFC for Chisel.

[mehnadnerd]

Closing, see chipsalliance/chisel#891 for followup.

[mehnadnerd]

Sagar reported an issue with excluding false.B (used when not excluding anything), I have filed an additional pull request to provide a no-argument fire which excludes nothing to provide a better solution. #1645

[mehnadnerd]

PR Merged, closing.