CoveragePkg: Adding the same bin twice is not an error (Should it be?)

[SkydiverTricky]

I have the following code:

cov.AddBins("Random Bins", 1, GenBin(0) );
cov.AddBins("Other Bins",  1, GenBin(0) );

while not cov.isCovered loop
  iv  := cov.randCovPoint;
  log("iv= (" & to_string(iv(0)) & ")");
  cov.ICover(iv);
end loop;

cov.writeBin;

And it gives the following output

KERNEL: %% Log   ALWAYS  in Default, iv= (0) at 0 ns
KERNEL: %% Log   ALWAYS  in Default, iv= (0) at 0 ns
KERNEL: %% WriteBin: 
KERNEL: %% Random Bins  Bin:(0)   Count = 1  AtLeast = 1
KERNEL: %% Other Bins  Bin:(0)   Count = 1  AtLeast = 1

This implies even though both bins cover the same value, they do not get both get covered at the same time, and have priority in the order they were created. Should it be an error to create two identical bins? or should it cover both bins when ICover is called?

You get the same behaviour with crossed bins, where the crosses are identical.

cov.AddCross("Random Bins", 1, GenBin(0), GenBin(0,7));
cov.AddCross("Other Bins", 1, GenBin(0) , GenBin(0,7));

[JimLewis]

I agree that what you are describing is erroneous (aka: a user error that is not currently detected by the coverage model). OTOH, detecting it would make entry an O(n**2) algorithm. For a larger model (AddCross supports crossing up to 20 items), this check time can be significant - see the discussion about Merging below.

There are a couple of things that will impact this issue, however, both are currently marked deprecated. First you can set the count mode to COUNT_ALL and when ICover is called, it will hunt for all bins that match the value. This will significantly increase search time when doing intelligent coverage randomization (because it does a COUNT_FIRST and it knows which bin to look in first since it randomly picked it - which unless you change it it is the bin to be counted).

   type CountModeType   is (COUNT_FIRST, COUNT_ALL) ;
    procedure SetCountMode (A : CountModeType) ;
    . . .
  Cov1.SetCountMode(COUNT_ALL) ; 

Another alternative is to SetMerging to true. SetMerging is supported
for removing count bins that are in a prior entered illegal or ignore bin.

    procedure SetMerging(A : boolean := TRUE ) ;
   . . . 
   Cov1.SetMerging(TRUE); 

Currently the feature of SetMerging that is deprecated is to merge a count bin that matches an existing count bin. Note that it does nothing though if they overlap but do not match. The reason this feature is deprecated is that it makes merging slow and results in slow capture of coverage models - OTOH maybe this needs to be looked at as those models were large (I think it was a cross with 20 items in the cross so maybe the test case was obnoxiously (and unrealistically) large as I have to test the full extent of the modeling capability).

If the check were required, then maybe Merging would be on by default and it would only be an error if Merging were turned off.

There are some checks like this that are like running a lint tool - they are nice things to do, but once you have checked it the first time, it would be nice to turn it off later during regressions.

[JimLewis]

In a way, this is like file reading during an image processing test. If the image file were corrupted, the test may fail after running for hours due to the error being detected in the image file.

Hence you have two options of protecting yourself:

1. As part of the test, first read the whole file just to check it and then close the file and run the test while reading the file
2. Create a separate program that checks the file and then always run the the program that checks the file one time before using it in a larger simulation.

While in my example I think choice 2 is the obvious solution, I don't think this issue has as obvious of a solution.

Thoughts?

[SkydiverTricky]

Like you said, this is mostly to cover user error, its probably overkill to try and catch it especially as the problem can explode as users add a few crosses. But Given Im only just getting into crosses, this may affect others more than me?

[JimLewis]

Based on my last class session, I added some additional materials to make the fact that it does a in order search of the data structure and picks the first matching bin a little more clear - in fact I think I even made sure to say it twice.

[SkydiverTricky]

Hi Jim I have another question following on from this. I have two coverage models, and the results are working well. But Im trying to fine tune the TB and Im wondering if it's possible to disable Bins. My 2nd coverage model gets to 100% much quicker than the first, and it has 5 bins. When this has 100 % cover, I want randCovPoint to return only the values 1 and 5, and not 2-4. Each bin was added with a separate AddBin function, but there doesnt appear to be an obvious DisableBin function. The only way I can think of to do it is manually outside of the coveragePType. Is there a way to do this? Thanks for any help - Merry Christmas and Happy New Year Richard

…

On Fri, 6 Dec 2019 at 14:19, JimLewis ***@***.***> wrote: I agree that what you are describing is erroneous (aka: a user error that is not currently detected by the coverage model). OTOH, detecting it would make entry an O(n**2) algorithm. For a larger model (AddCross supports crossing up to 20 items), this check time can be significant - see the discussion about Merging below. There are a couple of things that will impact this issue, however, both are currently marked deprecated. First you can set the count mode to COUNT_ALL and when ICover is called, it will hunt for all bins that match the value. This will significantly increase search time when doing intelligent coverage randomization (because it does a COUNT_FIRST and it knows which bin to look in first since it randomly picked it - which unless you change it it is the bin to be counted). type CountModeType is (COUNT_FIRST, COUNT_ALL) ; procedure SetCountMode (A : CountModeType) ; . . . Cov1.SetCountMode(COUNT_ALL) ; Another alternative is to SetMerging to true. SetMerging is supported for removing count bins that are in a prior entered illegal or ignore bin. procedure SetMerging(A : boolean := TRUE ) ; . . . Cov1.SetMerging(TRUE); Currently the feature of SetMerging that is deprecated is to merge a count bin that matches an existing count bin. Note that it does nothing though if they overlap but do not match. The reason this feature is deprecated is that it makes merging slow and results in slow capture of coverage models - OTOH maybe this needs to be looked at as those models were large (I think it was a cross with 20 items in the cross so maybe the test case was obnoxiously (and unrealistically) large as I have to test the full extent of the modeling capability). If the check were required, then maybe Merging would be on by default and it would only be an error if Merging were turned off. There are some checks like this that are like running a lint tool - they are nice things to do, but once you have checked it the first time, it would be nice to turn it off later during regressions. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#41?email_source=notifications&email_token=AJVPX26VC3TYONJX5E4CY5DQXJNPTA5CNFSM4JWUEZMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGEG23A#issuecomment-562589036>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJVPX26XNJMCEXLRWS7UWZTQXJNPTANCNFSM4JWUEZMA> .

[JimLewis]

With the current implementation of merging, you can enable merging and then add more to the bins:

Cov.SetMerging(TRUE);
Cov.AddBins(5, GenBin(1)) ; - do bin 1. 5 more times
Cov.AddBins(5, GenBin(5)); -- do bin 5, 5 more times

Note merging of a count bin into another count bin increased run time of some of my tests significantly and may be removed in the future - it is an O(n^2) algorithm rather and O(n).

OTOH, I am not sure if my test that was slowed down was a realistic and normal thing to do, so maybe it was unreasonably slow because the test was unreasonably complex - it iterated across a huge space ignoring most of it (to keep the model size reasonable).