Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Single Cycle CFU instruction issue with Vexriscv #747

Open
bala122 opened this issue Dec 10, 2022 · 10 comments
Open

Single Cycle CFU instruction issue with Vexriscv #747

bala122 opened this issue Dec 10, 2022 · 10 comments

Comments

@bala122
Copy link

bala122 commented Dec 10, 2022

Hi @tcal-x and @mithro ,
I'm experiencing issues with the working of the Vexriscv core, it's CFU bus and execution of single cycle CFU instructions.

Take the below case, where I have a bunch of single cycle cfu instructions running in a loop.

for loop{ 
	       //Based on flags, add offset or put 0
	       cfu_op1(2,0,0);
	       //Products
	       cfu_op1(3,0,0);
	       //Adding prods
	       cfu_op1(4,0,0);
	       //Store back and add into accum buffer
	       cfu_op1(5,0,0);
    }           

What Im seeing is that some of these instructions , say cfu_op1(3,0,0) are executing twice , even though I didn't intend it to. I have experienced this before with the multi-cycle cfu interface with the Vexriscv core. However, I assumed that the single-cycle cfu interface+ Vexriscv core would work. Do I pull the latest version of the g-cfu repository and have you guys come across this and resolved this bug or is this still there in the Vexriscv core?
Please suggest alternate methods to resolve this as well.

This is the cfu single cycle interface Im using:

  // Trivial handshaking for a combinational CFU
  assign rsp_valid = cmd_valid;
  assign cmd_ready = rsp_ready;

Thanks,
Bala.

@alanvgreen
Copy link
Collaborator

Hi Bala: have you seen this bug when running under Verilator? The trace files produced by Verilator are a great debugging tool for this kind of issue. If the problem is still not obvious then, could you please point to your code and post a picture of the traces (as seen in, say, gtkwave) across the CFU interface.

Thanks!

@bala122
Copy link
Author

bala122 commented Dec 20, 2022

Hi @alanvgreen ,
Thanks for the response. This was my mistake, I forgot to assert cmd_ready in my interface for different commands, so it was misbehaving.
Thanks,
Bala

@tcal-x
Copy link
Collaborator

tcal-x commented Dec 20, 2022

Hi @bala122 , sorry I missed this (thanks @alanvgreen !). I'm glad you figured it out. Did the waveform capture help with debugging the issue?

@bala122
Copy link
Author

bala122 commented Dec 24, 2022

Hi @tcal-x ,
I actually tested directly on synthesis because simulation took time for many layers before the given layer. However, yes upon feeding dummy values simulation could be useful.

@bala122
Copy link
Author

bala122 commented Jan 20, 2023

Hi @alanvgreen , Thanks for the response. This was my mistake, I forgot to assert cmd_ready in my interface for different commands, so it was misbehaving. Thanks, Bala

Hi @tcal-x and @alanvgreen ,
I just realized this issue still persists , when I'm getting back data from the CFU.
The weird part about this issue is that its working fine in simulation, however, after synthesis , its coming up. So, I cant even pull up the trace gdb files.
Also, when I replicate the cfu command to get back data from CFU to fetch again as shown below one after the other, it fetches it right and theres no issue.

cfu_cmd...
cfu_cmd..

I've checked my cfu interface as well, cmd_ready and cmd_valid are asserted. Im not sure if it's vivado or some issue in CFU itself.

I'll try with a multi-cycle interface and let you know if it persists.
Thanks,
Bala.

@bala122
Copy link
Author

bala122 commented Jan 22, 2023

Also @alanvgreen , @tcal-x ,
I just wanted to know, if some changes or big fixes to the cfu interface or Vexriscv been done since the last 6 months because I've not updated my local Google cfu repository since then.
Thanks,
Bala.

@tcal-x
Copy link
Collaborator

tcal-x commented Jan 23, 2023

Hi @bala122 -- when you say simulation, do you mean using Renode (make ...... renode) or Verilator (make PLATFORM=sim ..... load) ? Renode does not model the VexRiscv RTL so it is not useful for finding potential CFU interface handshaking issues.

You can look at the closed PRs on github to see what has been merged since your last update (unfortunately, it takes a bit more work now with all of the semi-automated Renode and TFLM updates).

If you were using Verilator simulatino and did NOT see the same error, then that does point to an implementation issue, possibly although unlikely a bug with Vivado, but more usually not meeting timing.

If you were using Renode, then there may be a logic error with the CFU interface, on either side. There may be an issue when a CFU instruction immediately follows a conditional branch -- it is worth checking the disassembly (build/software.elf.dis) to see if this occurs. If so, you can try inserting a no-op between the conditional branch and the CFU instruction. Here's an example of adding a no-op in the C/C++ code: https://github.com/google/CFU-Playground/blob/main/common/src/perf.cc#L147.

@bala122
Copy link
Author

bala122 commented Feb 5, 2023

Hi @tcal-x , @alanvgreen and @mithro ,
There are primarily two issues I've seen so far relating to a bug in the Vexriscv soft-core:-

  • If I just implement a complicated dataflow in one of the header files without synthesizing any cfu / accelerator peripherals, with just the Vexriscv core being synthesized, I see that it doesn't functionally work!!. This is kindof surprising given that I thought the Vexriscv core was bug-free in itself. I'll try to get some simulation data for this if possible in the future.
  • A main issue that's been constantly occurring is with the Vexrsicv CFU interface. I've noticed that in the cases where the following pattern occurs in dataflows that there's an issue:
    • Store fil-data, inp-data into CFU
    • Accelerated MAC compute and store into Accum regs
    • Getting back from accum regs: At this step, I see that the bus simply malfunctions and gives back '0' as data from CFU-accum regs although Im sure the functioning of the actual accel. module should be fine.

Please try to reproduce this issue (2nd one could be easier) and see if it occurs for you. I'll try to get some simulation data for this. Unfortunately, it takes a very long time to run the trace as well and at times I've seen that it gets stuck in simulation (hangs at cfu commands) while in synthesis it finishes with the above bus malfunction. Anyway, I'll try to get some trace data for this.

Thanks,
Bala.

@bala122
Copy link
Author

bala122 commented Feb 6, 2023

Hi @tcal-x , @alanvgreen and @mithro , There are primarily two issues I've seen so far relating to a bug in the Vexriscv soft-core:-

* If I just implement a complicated dataflow in one of the header files without synthesizing any cfu / accelerator peripherals, with just the Vexriscv core being synthesized, I see that it doesn't functionally work!!. This is kindof surprising given that I thought the Vexriscv core was bug-free in itself. I'll try to get some simulation data for this if possible in the future.

* A main issue that's been constantly occurring is with the Vexrsicv CFU interface. I've noticed that in the cases where the following pattern occurs in dataflows that there's an issue:
  
  * Store fil-data, inp-data into CFU
  * Accelerated MAC compute and store into Accum regs
  * Getting back from accum regs: At this step, I see that the bus simply malfunctions and gives back '0' as data from CFU-accum regs although Im sure the functioning of the actual accel. module should be fine.

Please try to reproduce this issue (2nd one could be easier) and see if it occurs for you. I'll try to get some simulation data for this. Unfortunately, it takes a very long time to run the trace as well and at times I've seen that it gets stuck in simulation (hangs at cfu commands) while in synthesis it finishes with the above bus malfunction. Anyway, I'll try to get some trace data for this.

Thanks, Bala.

I'll have to confirm the 2nd issue again- I'm sorry, I'm guessing it was a design issue regarding my CFU instruction decoding, where upon examining the trace, I've noticed it resetting all my accum regs often when I didn't intend to. However, I've still seen instances of bus issues a year back when I was performing a verilator simulation and I had to run the cfu command twice in order to get back accumulator values.
Anyways, I'll confirm if I find an actual issue soon. The first issue was still baffling to me though.
Thanks,
Bala.

@tcal-x
Copy link
Collaborator

tcal-x commented Feb 6, 2023

@bala122 thanks for the update; I'm trying to find time to take a closer look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants