Revert removal of bit extraction const prop for literals

[jackkoenig]

#820 removed what looked like vestigial code but it turns out to be a useful API that is being used in rocket-chip and elsewhere. This PR restores the ability to do "out of range" extractions on literals. FIRRTL does not allow out-of-range extraction so something like:

0.U(10) // === false.B

became a FIRRTL error post #820.

This PR includes tests that illustrate behavior that worked before #820 and in this PR but does not work on current master.

Related issue:

Type of change: bug fix

Impact: | API addition (no impact on existing code)/restoration

Development Phase: implementation

Release Notes

[ducky64]

I think this is very much a hacky solution to preserve a previous edge-case API. I think the proper fix, if we want to preserve this behavior, would be to enable this in FIRRTL. I personally think an out-of-range slice should be an error, since it could be indicative of a programming error, and it can be overridden by adding a sufficiently long explicit width specificier to the literal.

[ducky64]

(the other thing we could do, I guess, would be to do const prop in chisel frontend, though that seems architecturally ugly, and practically would require effort to keep chisel frontend in sync with firrtl and likely end up with subtle differences in behavior depending on whether chisel or firrtl does some particular computation)

[jackkoenig]

I'm somewhat sympathetic to the idea that an out of range slice of a literal with a defined width should be an error, but I think it's pretty restrictive to disallow it on something without a defined width.

For example, from rocket-chip we have:

object SBErrorCode extends scala.Enumeration {
  type SBErrorCode = Value
  val NoError    = Value(0)
  val Timeout    = Value(1)
  val BadAddr    = Value(2)
  val AlgnError  = Value(3)
  val BadAccess  = Value(4)
  val OtherError = Value(7)
}
import SBErrorCode._
...
      for (i <- 0 until 3)
        sbErrorReg(i) := Mux(SBCSWrEn && SBCSWrData.sberror(i) === 1.U, NoError.id.U(i), // W1C
                         Mux((sb2tl.module.io.wrEn && !sb2tl.module.io.wrLegal) || (sb2tl.module.io.rdEn && !sb2tl.module.io.rdLegal), BadAddr.id.U(i), // Bad address accessed
                         Mux((tryWrEn || tryRdEn) && sbAlignmentError, AlgnError.id.U(i), // Address alignment error
                         Mux((tryWrEn || tryRdEn) && sbAccessError, BadAccess.id.U(i), // Access size error
                         Mux((sb2tl.module.io.rdDone || sb2tl.module.io.wrDone) && sb2tl.module.io.respError, OtherError.id.U(i), sbErrorReg(i)))))) // Response error from TL

Ignoring some of the complex logic, this makes use of the enumerated values and indexes them from 0 to 3 eg. AlgnError.id.U(i)

I think that being able to index arbitrary bits of a literal when there is no defined width makes a lot of sense since it's treating the literal as a numerical constant rather than a constant that automatically decides to have the minimum necessary width.

[jackkoenig]

I agree that the current implementation is "hacky" or at least weird in that it's the only constant propagation going on in Chisel, but if we're going to remove APIs we should do it thoughtfully and not because the implementation isn't clean.

I do think it could be cool if we constructed FIRRTL IR nodes and leverage FIRRTL's const prop as the nodes are constructed in Chisel, and that would certainly be more general and robust but this PR is about restoring an in-use API

[aswaterman]

Agreed - 0.U(10) should work, whereas (0.U(1.W))(10) could be treated as an out-of-range extract.

[ducky64]

I think Chisel behavior should be consistent with FIRRTL behavior, but if there's agreement on changing FIRRTL semantics to @aswaterman 's proposal (which seems sane-ish) I'm happy merging this as a hotfix if the FIRRTL changes are expected to take longer.

[ducky64]

Can you add a comment on the implementation and tests to the effect of "this preserves compatibility for an edge case of existing behavior, but is currently under debate, this may end up as part of a deprecation schedule", maybe also including a link to a tracking issue? I'd like to make it clear that this is a hotfix, so we don't forget the context if the clean fix ends up tabled for a few months.

Otherwise, I'm happy with the hotfix.

[jackkoenig]

Yep, sounds good, I'll file an issue and add a comment pointing to it

[jackkoenig]

@ducky64 done

[ducky64]

lgtm