Repository navigation
Be more careful with variants in OneShotCfg.gen_results - #159
Conversation
6459cc2 to
489ef55
Compare
| # For the latter check, we don't necessarily know that item (the | ||
| # configuration that is being run) is a type that defines variant | ||
| # at all. If not, we just interpret the variant as None. | ||
| item_variant = getattr(item, "variant", None) |
There was a problem hiding this comment.
Question: do you think variants are a ubiquitous enough concept in DVSim that we should just define self.variant: str | None = None in FlowCfg.__init__? At least for the sim code I've seen them used a lot, but maybe they're just very sim-specific?
From the examples I've seen, such as 32kB and 64kB OpenTitan rom_ctrl variants, it seems like it should be part of any flow?
There was a problem hiding this comment.
Hmm, I think they are! Historically, I suspect they got bolted in to whichever bits of dvsim needed them when the variants feature was added, but there's no reason that we shouldn't structure things sensibly now.
I've opened the cleanup issue as #165.
489ef55 to
900d702
Compare
900d702 to
43d95f5
Compare
We don't necessarily know that all sub-configurations are of type SimCfg. Indeed, this failed for me at runtime when trying an FPV test. We also don't know (from the types) that the sub-configuration defines _gen_results, because self.cfgs is a list of the base FlowCfg type. Add a runtime check to make sure the elements are in fact OneShotCfg instances. Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
43d95f5 to
9fcca80
Compare
machshev
left a comment
machshev
left a comment
There was a problem hiding this comment.
LGTM! Thanks @rswarbrick
We don't necessarily know that all sub-configurations are of type SimCfg. Indeed, this failed for me at runtime when trying an FPV test.
We also don't know (from the types) that the sub-configuration defines _gen_results. Practically speaking, it does, so let's just move this to an explicit runtime check, which quiets the type checker.