chiseltest icon indicating copy to clipboard operation
chiseltest copied to clipboard

Allow expectDequeue with partial BundleLiterals?

Open zhuanhao-wu opened this issue 5 years ago • 7 comments

When testing using the DecoupledDriver, I find that sometimes I only care about the values of certain fields of a Bundle in some portion of the tests and don't care about other fields.

Currently, It seems I have to specify the full field list in the Lit() or the test will fail in expectDequeue or expectDequeueNow.

I wonder if having an expectDequeuePartial checking present fields only is possible? Or I'd better provide the full list or have a separate helper?

zhuanhao-wu avatar Jan 08 '20 21:01 zhuanhao-wu

Yeah, that's an interesting question. I'm not sure I want to have expectDequeuePartial because of the potential to proliferate into all kinds of special-case expectDequeue* which could get really confusing.

It might make sense to support partial bundle literals in expect directly, especially since it doesn't drop user data. But on the down side, there's less protection against the user forgetting to specify something.

Thoughts?

ducky64 avatar Jan 08 '20 22:01 ducky64

@zhuanhao-wu In the short run, there's the option of writing a specific helper function for your usecase.

edwardcwang avatar Jan 08 '20 23:01 edwardcwang

@ducky64 From a user's standpoint, one way I can think of is to add an argument to expect like allowPartialBundle to control the matching behavior. Another way is to have something like

test(new MyModule) allowing PartialBundle { c => 
    ... 
}

@edwardcwang Yeah thanks, I can do that in my project.

zhuanhao-wu avatar Jan 08 '20 23:01 zhuanhao-wu

I’m also interested in the use case. Would default initialization to (something like) BitPat be plausible here?

kammoh avatar Jan 09 '20 00:01 kammoh

@kammoh As a workaround, I think you can have an implicit class that adds expectPartial() to a bundle and skips unspecified fields of the bundle.

  implicit class BundlePartialMatcher[T <: Bundle](x: T) {
    def expectPartial(data: T): Unit = expectPartialWithStale(data, None, false)

    def expectPartialWithStale(value: T, message: Option[String], stale: Boolean): Unit = {
      // both must be bundle
      require(x.elements.keys == value.elements.keys)
      (x.elements zip value.elements) foreach {
        case ((_, x), (_, value)) => {
          value.litOption match {
            case None => Unit // continue because we don't need to match field that is not present
            case Some(_) =>
              x match {
                case xx: Bundle => xx.expectPartial(value.asInstanceOf[Bundle], stale) // we recursively do partial match
                case _ => x.expect(value) // normal match on normal data
              }
          }
        }
      }
    }
  }

zhuanhao-wu avatar Jan 09 '20 01:01 zhuanhao-wu

That's amazing... and also simultaneously kind of terrifying.

One problem I see with making partial bundles a top-level option is that it's not an obvious option, and library builders might need to be aware of this global flag. I think it's worth reasoning through whether we should enable partial bundle support everywhere - I think if it's not terrible, that would be the preferred option for simplicity.

I think the reason it's strict now is because it's easier to expand an API than to remove it. This might be a good case to expand it.

ducky64 avatar Jan 10 '20 22:01 ducky64

An old issue but still a wanted one :) If you use DontCare in your bundles, it doesn't seem like there's a nice way to test the interface without gluing together a bunch of helpers (which, fwiw, does at least work though some tweaks were needed):

    implicit class BundlePartialMatcher[T <: Bundle](x: T) {
        def expectPartial(data: T): Unit =
            expectPartialWithStale(data, None, false)

        def expectPartialWithStale(
                value: T, message: Option[String], stale: Boolean): Unit = {
            // both must be bundle
            require(x.elements.keys == value.elements.keys)
            (x.elements zip value.elements) foreach {
                case ((_, x), (_, value)) => {
                    value.litOption match {
                        // continue because we don't need to match field that
                        // is not present
                        case None => ()
                        case Some(_) =>
                            x match {
                                case xx: Bundle =>
                                    // we recursively do partial match
                                    xx.expectPartialWithStale(
                                        value.asInstanceOf[Bundle],
                                        None,
                                        stale
                                    )
                                case _ =>
                                    // normal match on normal data
                                    x.expect(value)
                          }
                    }
                }
            }
        }
    }

    implicit class ValidDriverSweets[T <: Bundle](x: ValidIO[T]) {
        def expectDequeuePartial(data:T):Unit = {
            fork.withRegion(Monitor) {
                x.waitForValid()
                x.valid.expect(true.B)
                x.bits.expectPartial(data)
            }.joinAndStep()
        }
    }

ezhes avatar Feb 18 '24 04:02 ezhes