NYU-Processor-Design / nyu-core

The code and tests for the RISCV-32I compatible core for the NYU Processor Design VIP team.
Creative Commons Zero v1.0 Universal
2 stars 12 forks source link

test(Alu): Fix alu test, demonstrate testing style #148

Closed nickelpro closed 5 months ago

nickelpro commented 5 months ago

So this fixes just the ALU tests, because fixing all the tests will take awhile and it's more important to demonstrate how to do this.

Notable stuff:

nyu::getDUT<>

You need to grab references to a given device using:

#include <NyuTestUtil.hpp>

static void some_test_function() {
   auto& device {nyu::getDUT<VDevice>()};
   // ...
}

The reason for this is because of the way the test runner and the coverage reporter interact. The coverage reporter expects the lifetime of the device to be longer than the surrounding test-case, nyu::getDUT ensures this happens.

nyu::eval()

Similarly, you should be using nyu::eval() instead of device.eval(). This is because when using "tracers" instead of devices, there's no device method to call and you need to use the freestanding nyu::eval() overload. For uniformity (and because it greatly simplifies debugging), always use nyu::eval().

This is actually fixed now and either style works, there's no advantage to nyu::eval() except the consistency with nyu::reset() and nyu::tick() and the case mentioned below.

This has some advantages as well, if you want to call .eval() 5 times, the overload lets you do that directly:

// Before
for(int i=0; i < 5; ++i)
  device.eval();

// After
nyu::eval(device, 5);

Large input space coverage

Most of the tests use random integers where doing so is inappropriate and leads to "random" amounts of coverage being reported which can go up or down a few percent. This is wrong, and should never be the only mechanism to test a device. At the least, each bit of the input space should be tested in isolation so that the coverage is always reported consistently. The ALU tests now demonstrate how to do that, as with the following:

for(std::uint32_t opA {1}; opA; opA <<= 1)
  for(std::uint32_t opB {1}; opB; opB <<= 1)
    run_a_test(opA, opB);
nickelpro commented 5 months ago

This has been rebased onto #147

The reason is that correctly testing the ALU revealed several bugs in the existing version, notably both SLTs are wrong and instead perform <=, the ARS also worked incorrectly for reasons I didn't decode

codecov[bot] commented 5 months ago

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (e74b8c0) 97.86% compared to head (3213819) 98.18%.

Files Patch % Lines
rtl/Branch_Predictor.sv 87.50% 2 Missing :warning:
rtl/IDEX.sv 95.65% 1 Missing :warning:
rtl/Pipeline_Reset.sv 92.30% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #148 +/- ## ========================================== + Coverage 97.86% 98.18% +0.32% ========================================== Files 13 13 Lines 234 276 +42 ========================================== + Hits 229 271 +42 Misses 5 5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.