fvutils / pyvsc

Python packages providing a library for Verification Stimulus and Coverage
https://fvutils.github.io/pyvsc
Apache License 2.0
113 stars 26 forks source link

Unusual dist behavior regarding constraint conflicts #63

Closed junechanh closed 3 years ago

junechanh commented 3 years ago

I've been playing around with dist constraints to see how they react in various scenarios, and I found some cases wherein they yield unexpected/unintuitive behavior. My code is similar to that of issue #61 :

import vsc

@vsc.randobj
class my_item(object):
    def __init__(self):
        self.a = vsc.rand_bit_t(8)

    @vsc.constraint
    def valid_ab_c(self):
        pass
        # vsc.soft(self.a < 5) #Case A: this is fine since it is a looser bound than the dist constraint
        # vsc.soft(self.a > 5) #Case B: this throws off distribution; would have expected this soft constraint to be ignored as if it were commented out and distribution to follow 1:97:1:1)
        # self.a > 5 #Case C: this causes a constraint error since this value domain is disjoint from that of the dist constraint

    @vsc.constraint
    def dist_a(self):
        vsc.dist(self.a, [
            vsc.weight(0,  1),
            vsc.weight(1,  97),
            vsc.weight(2,  1),
            vsc.weight(3,  1),
            #vsc.weight(20, 10000) #Case D: this throws off distribution when combined with the line labeled below (the larger the weight, the worse -- seems that the weights get redistributed unevenly)
            ])

hist = {}
total_cnt = 0

def add_to_hist(val):
    global hist
    global total_cnt
    total_cnt += 1
    if val not in hist: hist[val] = 0
    hist[val] += 1

def print_dist(val):
    global hist
    global total_cnt
    print(f"{val}: {hist[val]} " + "(%.1f%%)" %(hist[val] / total_cnt * 100))

item = my_item()
for i in range(1000):
    with item.randomize_with() as it:
        pass
        #it.a <= 5 #Case D
    add_to_hist(item.a)

for key in sorted(hist.keys()):
    print_dist(key)

The cases I wanted to highlight are cases B and D:

Case B: I am getting a distribution of mostly '1' and '2', ~45% and ~52%, respectively. I might have expected ~97% and ~1% if the dist constraint takes precedence, or an even distribution if the soft constraint takes precedence.

Case D: I am getting a distribution of mostly '0' and '2', ~50% and ~48%, respectively. I might have expected ~1% each if weights of the invalid/unused portions of the dist domain are ignored, or near-even if they are redistributed to the valid portions of the domain.

mballance commented 3 years ago

Hello @junechanh, I've been looking into dist and soft constraints, and believe I have improvements to the 'B' case. You are correct that, previously, dist constraints and soft constraints were given roughly the same priority. This allowed soft constraints to skew the distribution from dist constraints. I've improved that in the 0.3.7 release of PyVSC. Case 'D' is a bit more problematic. The PyVSC implementation of dist constraints selects the target range based solely on the weights -- in other words, it assumes that all ranges are reachable. In case 'D', the target value 20 will be selected according to its very high weight. When that value cannot be selected, one of the reachable values will be selected using unweighted-random selection. This does match SystemVerilog semantics (whether or not it's desirable in this case):

Absent any other constraints, the probability that the expression matches any value in the list is proportional
to its specified weight. If there are constraints on some expressions that cause the distribution weights on
these expressions to be not satisfiable, implementations are only required to satisfy the constraints. An
exception to this rule is a weight of zero, which is treated as a constraint.

I'll leave this open for now until you're able to confirm the improvements in 0.3.7.

Best Regards, Matthew

junechanh commented 3 years ago

Thanks for the support on this Matthew. I wasn't aware of the behavior for case D in SystemVerilog; I appreciate the education. I can confirm that the scenario in case B is fixed with the latest. I'll close this issue now.