[dv] Unpack some randomised structs in DV code #28581
Conversation
rswarbrick
force-pushed
the
unpacked-dv-structs
branch
4 times, most recently
from
1f62c2a to
a8bbca8
Compare
This won't really matter, but the unnecessary "packed" shows that I didn't really understand the difference between packed and unpacked structs in SystemVerilog when I wrote the code in 2022. Or indeed yesterday morning... Signed-off-by: Rupert Swarbrick <[email protected]>
rswarbrick
force-pushed
the
unpacked-dv-structs
branch
from
a8bbca8 to
ef6bf43
Compare
|
Well, this took rather longer than I hoped! I'm optimistic that everything should work (at last) with this newest version. In particular, the |
rswarbrick
requested review from
hcallahan-lowrisc
and removed request for
a team
For example, consider flash_mp_region_cfg_t. Structs of this type are
randomised in several places (see flash_ctrl_error_mp_vseq, for
example). This isn't going to do what we need if the struct is packed.
For example (without further constraints), each of those seven mubi4_t
elements is going to be have an invalid encoding with probability
14/16.
This might be worked around. Indeed, I see that
flash_ctrl_error_mp_vseq has explicit constraints for the regions,
that will force the fields to take named enum values. But it's
probably more sensible to default to more guessable behaviour.
As well as flash_ctrl_error_mp_vseq, this commit makes the same change
to:
- flash_bank_mp_info_page_cfg_t (another 7 mubi values, with the
same analysis).
- flash_op_t (contains several enums and gets confusingly randomised
in several places, which is why I noticed this in the first place)
- rd_cache_t (contains an enum variable)
It turns out that removing "packed" isn't quite enough. The biggest
reason is that there were quite a few "solve before" lines that aren't
actually allowed if you have something more interesting than a bit
vector.
Changes needed:
- Make the fields of flash_op_t themselves be rand. This is needed
because calling my_class.randomise(some_class_variable) doesn't
work if some_class_variable is an unpacked struct whose fields
aren't explicitly marked rand.
- This is also needed in flash_mp_region_cfg_t and
flash_bank_mp_info_page_cfg_t. For some reason, *that* isn't
visible at compile time, but you end up with a runtime error where
something is randomised with X values (not allowed by the spec!)
and the root cause is that we're not actually randomising e.g. a
region config struct, so it gets its initial value (of X).
- Rephrase lots of rules of the form "solve xyz before
flash_op" to things like "solve xyz before flash_op.addr". This is
because the SystemVerilog spec doesn't allow anything except an
integer in as a thing to be solved before/after. I think the DV
engineer just has to pretend to be a compiler. Sigh...
- Remove some ordering constraints of the form "solve en_mp_regions
before mp_regions". This is definitely not allowed (because
mp_regions is an array of unpacked structs, so not a bit vector).
I'm not completely convinced we care but, if we do, my proposal
would be to randomise the items of the vector explicitly as we get
to them ("late randomisation"). The code ends up simpler, I think.
- Fix flat-out bogus syntax in flash_ctrl_invalid_op_vseq.sv.
Antonio and I have fixed the same error in quite a few places in
the past. In this case, it came from commit 9b01461 in 2022.
Signed-off-by: Rupert Swarbrick <[email protected]>
The layout of this structure doesn't really matter and it gets randomised (see e.g. adc_ctrl_env_cfg::filter_cfg), which will mean that min_v and max_v are picked uniformly from [2^31, 2^31-1]. I really don't believe that's intended. Signed-off-by: Rupert Swarbrick <[email protected]>
This is inspired by trying to solve randomisation errors. It turns out that randomising values that aren't in a *static* structure causes some exciting problems with "solve before" constraints. In hindsight, I think this change isn't quite needed: I could have bodged something into the existing code. But I think it's probably an improvement anyway (and I've got it working!). One change is to make some of the class variable names a bit clearer. For example, the "cond" field in adc_chn*_filter_ctl_* controls whether the filter matches inside the range or outside of it. I actually guessed wrong at first, but I think this makes a strong argument for making the code more explicit on the DV side. That is now reflected with a variable called "match_outside" (instead of my original guess of "match_inside", which is exactly backwards!) Indeed, I've just checked again and looked at `theory_op_operation.md`. That document *does* mention a field called "cond" and that it controls whether the filter matches inside or outside of the range. It *doesn't* say which... This clearly needs tidying up, but I think that sort of work can be folded into the next proper release. Signed-off-by: Rupert Swarbrick <[email protected]>
rswarbrick
force-pushed
the
unpacked-dv-structs
branch
from
ef6bf43 to
fa614b7
Compare
| uint num_pages; // 0:NumPages % start_page | ||
| uint start_page; // 0:NumPages-1 | ||
| typedef struct { | ||
| rand mubi4_t en; // enable this region |
There was a problem hiding this comment.
Why is the rand required of this struct?
As far as I understand, this will now make sure that randomization will choose a legal value (MuBi4True/False)?
But do we loose coverage here if we don't generate illegal values?
There was a problem hiding this comment.
It's needed because SystemVerilog randomises an unpacked struct by randomising each of its rand fields. It's not massively obvious in the spec, but the behaviour is defined in section 18.4 (page 507)
The point is that I might have a class variable that is a struct. If I mark that variable as rand, this tells SystemVerilog that I'd like to randomise the structure. In order to do so, SystemVerilog needs to know which fields need randomising. As with objects, structs default to fields not getting randomised. Here, I would like all of the fields to be randomised, so I have to say so explicitly.
You're right: randomisation will now choose legal mubi4_t values.
I'm unconvinced that we'll lose any meaningful coverage with the change. If we do then it's probably good to find out! Without the change, 14/16 structures will have bogus enum values. That probably isn't a very efficient way to test the behaviour of code that uses the structure... :-)
There was a problem hiding this comment.
ok sounds good. thanks for the explanation!
|
For Thinking about it, I haven't do the same for |
|
For the |
|
yeah I know:) here is the status of this weeks CI run: |
2 participants
This was triggered by a rather long hunt yesterday evening trying to figure out why a variable of type
flash_op_twas randomising to a silly value inflash_ctrl_error_mp_vseq. This was quite a SystemVerilog learning experience for me!I now know that randomising a packed struct in SystemVerilog is essentially the same as randomising a bit vector and then casting the result back to the structure type. In particular, this will merrily trash any enum fields.
I think this PR catches all the similar occurrences in the codebase: I manually hunted through the hits in
git grep 'struct packed' -- '*dv*.sv*'and I don't think I've missed anything.