[dv,cip] Rewrite `_DV_MUBI_DIST to avoid warnings#29330
[dv,cip] Rewrite `_DV_MUBI_DIST to avoid warnings#29330rswarbrick wants to merge 1 commit intolowRISC:masterfrom
Conversation
rswarbrick
force-pushed
the
dv-mubi-dist
branch
from
dedcac0 to
80bf0aa
Compare
|
Dammit! Just realised that I had an off-by-one error in the commit message. Take |
rswarbrick
force-pushed
the
dv-mubi-dist
branch
from
80bf0aa to
c6afdee
Compare
marnovandermaas
left a comment
marnovandermaas
left a comment
There was a problem hiding this comment.
I think it's not quite right, so I've made a suggestion.
rswarbrick
force-pushed
the
dv-mubi-dist
branch
from
c6afdee to
837e8c7
Compare
elliotb-lowrisc
left a comment
elliotb-lowrisc
left a comment
There was a problem hiding this comment.
I'm confused. Why not use the :/ operator for the other values?
|
The problem is just that you might have the "other" category broken into multiple ranges and you don't want the probability of a given "other" value to depend on the length of the range that it happens to be in. |
elliotb-lowrisc
left a comment
elliotb-lowrisc
left a comment
There was a problem hiding this comment.
Ah OK, I think some tweaks to the code comments could avoid some future confusion
It turns out that randomising with a distribution like this
if (0) {
XYZ dist { [1:0] := 1; };
}
generates a runtime warning from Xcelium (even though the distribution
expression isn't actually used).
Fortunately, range endpoints can be general expressions, so we can do
everything with ternary operators instead. We can also avoid repeating
stuff if we define `MIN / `MAX macros.
This commit also fixes the weighting for true/false. Suppose we want
to distribute with weights ZeroW, OneW, OtherW for 0, 1, other in a
way that e.g. the fraction of ones is OneW / (ZeroW + OneW + OtherW).
The following would definitely be wrong:
value dist { 0 := ZeroW,
1 := OneW,
[2:9] := OtherW };
Because there are eight "other" values, the fractions end up looking
like OneW / (ZeroW + OneW + 8 * OtherW). Oops!
To make everything line up properly, we multiply by 8:
value dist { 0 := 8 * ZeroW,
1 := 8 * OneW,
[2:9] := OtherW };
and will now have fractions like
8 * OneW / (8 * ZeroW + 8 * OneW + 8 * OtherW
=
OneW / (ZeroW + OneW + OtherW)
Generalising to the macro, there are (MAX_ - 1) possible
values (instead of 10), so we need to scale up by (MAX_ - 3).
Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
|
Whoops, wrong button |
marnovandermaas
left a comment
marnovandermaas
left a comment
There was a problem hiding this comment.
This looks good to me thanks for the clarifying comments.
vogelpi
left a comment
vogelpi
left a comment
There was a problem hiding this comment.
Thanks @rswarbrick for cleaning this up. The comments and motivation are very clear. But I have some questions regarding the correctness.
| TRUE_ := (T_WEIGHT_) * ((MAX_) - 3), \ | ||
| FALSE_ := (F_WEIGHT_) * ((MAX_) - 3), \ |
There was a problem hiding this comment.
I am not sure the -3 is correct. In the example you max in the commit message, MAX_ is 9, but you scale by 8 (== MAX_ - 1) and not by 6 (== MAX_ - 3).
| [0 : `_DV_TERNARY_MIN(TRUE_, FALSE_)-1] := (OTHER_WEIGHT_), \ | ||
| [(`_DV_TERNARY_MIN(TRUE_, FALSE_)+1) : \ | ||
| (`_DV_TERNARY_MAX(TRUE_, FALSE_)-1)] := (OTHER_WEIGHT_), \ | ||
| [(`_DV_TERNARY_MAX(TRUE_, FALSE_)+1):(MAX_)] := (OTHER_WEIGHT_) \ |
There was a problem hiding this comment.
I don't think this is correct based on an example I just did on paper based on the Mubi4 type with equal weights for true, false and invalid. Unless I made a mistake, inserting the numbers gives me the following weights:
- 6 (true) := 4
- 9 (false) := 4
- 0:5 := 1/3 - meaning the total weight for this group is 6 * 1/3 = 2
- 7:8 := 1/3 - total weight 2 * 1/3 = 2/3
- 10:15 := 1/3 - total weight 6 * 1/3 = 2
So the total weight of the invalid group is 4 + 2/3 and not 4. Does this make sense?
I think you need to take the set size into account as well and then use :/ instead of :=.
|
One more point that came to my mind: this change is going to affect potentially all regressions. So we must keep an eye on the coverage metrics and make sure results won't degrade @rswarbrick . Maybe to reduce the risk, can we infer which blocks are expected to be affected? I.e., where is macro used / not used? |
4 participants
It turns out that randomising with a distribution like this
generates a runtime warning from Xcelium (even though the distribution expression isn't actually used).
Fortunately, range endpoints can be general expressions, so we can do everything with ternary operators instead. We can also avoid repeating stuff if we define MIN / MAX macros.
This commit also fixes the weighting for true/false. Suppose we want to distribute with weights ZeroW, OneW, OtherW for 0, 1, other in a way that e.g. the fraction of ones is OneW / (ZeroW + OneW + OtherW).
The following would definitely be wrong:
Because there are eight "other" values, the fractions end up looking like OneW / (ZeroW + OneW + 8 * OtherW). Oops!
To make everything line up properly, we multiply by 8:
and will now have fractions like
Generalising to the macro, there are (MAX_ - 1) possible values (instead of 10), so we need to scale up by (MAX_ - 3).