Instruction Interface not stable assertion failed

[huettern]

After the change of @SamuelRiedel in pulp-platform/snitch#69, i get assertion failures:

# ** Error: [ASSERT FAILED] [tb_bin.i_dut.i_snitch_cluster.i_cluster.gen_core[8].i_snitch_cc.i_snitch.InstructionInterfaceStable] InstructionInterfaceStable (/home/noah/git/snitch-dace/snitch/hw/ip/snitch/src/snitch.sv:2599)
#    Time: 350 ns Started: 349 ns  Scope: tb_bin.i_dut.i_snitch_cluster.i_cluster.gen_core[8].i_snitch_cc.i_snitch.InstructionInterfaceStable File: /home/noah/git/snitch-dace/snitch/hw/ip/snitch/src/snitch.sv Line: 2599
# ** Error: [ASSERT FAILED] [tb_bin.i_dut.i_snitch_cluster.i_cluster.gen_core[0].i_snitch_cc.i_snitch.InstructionInterfaceStable] InstructionInterfaceStable (/home/noah/git/snitch-dace/snitch/hw/ip/snitch/src/snitch.sv:2599)
#    Time: 361 ns Started: 360 ns  Scope: tb_bin.i_dut.i_snitch_cluster.i_cluster.gen_core[0].i_snitch_cc.i_snitch.InstructionInterfaceStable File: /home/noah/git/snitch-dace/snitch/hw/ip/snitch/src/snitch.sv Line: 2599
# ** Error: [ASSERT FAILED] [tb_bin.i_dut.i_snitch_cluster.i_cluster.gen_core[4].i_snitch_cc.i_snitch.InstructionInterfaceStable] InstructionInterfaceStable (/home/noah/git/snitch-dace/snitch/hw/ip/snitch/src/snitch.sv:2599)
#    Time: 418 ns Started: 417 ns  Scope: tb_bin.i_dut.i_snitch_cluster.i_cluster.gen_core[4].i_snitch_cc.i_snitch.InstructionInterfaceStable File: /home/noah/git/snitch-dace/snitch/hw/ip/snitch/src/snitch.sv Line: 2599

Can you reproduce this using the attached binary? Is it a concern?

ssr_intrinsic.tar.gz

[zarubaf]

Thanks for the issue. I'll try digging into it over the weekend. The assertion shouldn't trigger, maybe we need to re-investigate the pre-fetching policy a bit further. I'll keep you posted.

[SamuelRiedel]

Thanks Noah for the report and for including a binary. I looked into it and as far as I understand, the problem is that the instructions triggering these assertions come from the bootrom, which is set to be non-cacheable. They bypass the cache and the ready signal is only asserted for one cycle while Snitch has to wait for an outstanding load.

So either we change the assertion to not check non-cacheable instructions, but then we have no guarantee that they will always be stable. Or we change the bypass logic to keep the ready and the instruction stable.

[zarubaf]

Of course @SamuelRiedel is right! At first glance it seems that this is actually a bit more problematic to handle than I would wish for.

• I think the same problem that was exhibited by the instruction cache pre-fetcher is also problematic here. I don't see any reason why it wouldn't also be erroneous to do so on non-cacheable regions. Right, @SamuelRiedel?
• If we would keep the request stable, the bypass logic would probably need a couple of more bits to a. store the instruction address b. the instruction word. Otherwise we can't really distinguish whether this is a new request or still the old one I guess.
• The whole bypass handling is pretty messy as it is now. Interestingly, we would already have the required flip-flops in the L0 buffer to handle this scenario. We could potentially implement a straight-forward evict logic in case we find that the region is non-cacheable. I didn't opt for such an implementation in the first place because we didn't have a proper testbench in place for the L0. In the meantime that has also changed.
• For the moment we could disable the assertion when fetching from non-cacheable region, but leave a todo there to re-visit this (sooner than later).

What's your take on this @noah95 @SamuelRiedel?

[SamuelRiedel]

• I agree, I think all instructions should be stable at the interface, not just the cacheable ones.
• Reusing the resources of the L0 sounds reasonable. I think this should be the long-term solution. Disabling the assertion for now and creating an issue sounds good to me.

[huettern]

Thank you for investigating this. Since I'm not familiar with the instruction fetch and cache at all I can't contribute to the discussion, sorry. But let me know if I can help testing any fixes.