Ifu cache update #769
Ifu cache update #769
Conversation
source/ifu/ifu_cache.sv
Outdated
| import ifu_pkg::*; | ||
|
|
||
| #( | ||
| parameter NUM_TAGS, // Number of tags |
There was a problem hiding this comment.
Parameters that we don't expect to override in the instance could and should be part of the ifu package
There was a problem hiding this comment.
as amichai said, Please move the parameters into ifu_package. Its not an error its a preferred coding style in mafia
source/ifu/ifu_cache.sv
Outdated
| /////////////// | ||
| // Utilities // | ||
| /////////////// | ||
| task updatePLruTree(inout logic [NUM_LINES - 2 : 0 ] tree , input int line ); |
There was a problem hiding this comment.
This is not code that is RTL compatible..
There was a problem hiding this comment.
please use a function instead or macros if possible. as Amichai posted. Tasks can be synthesizable if you write synthesizable code inside them. Task may work but lets not use them for proper coding style
source/ifu/ifu_cache.sv
Outdated
| endtask | ||
|
|
||
|
|
||
| function int getPLRUIndex(logic [NUM_LINES - 2 : 0 ] tree); |
There was a problem hiding this comment.
This could be compatible with RTL, but I do not recommend..
Macros or module are better options
source/ifu/ifu_cache.sv
Outdated
| if (freeLine == -1)begin | ||
| freeLine = getPLRUIndex(plruTree); | ||
| end | ||
| dataArray[freeLine] = mem_rspInsLineIn; |
There was a problem hiding this comment.
Looking at the Code seems like there are lots of latches that are not intended..
You should have explicit DFF for the arrays.
There was a problem hiding this comment.
To avoid latches, give some default value at the beginning of the always block (let me know if I was clear). Additionally, if you use always_comb with inferred latches Quartus will give you an error
and updatePLRUTree to submodules instead of tasks and functions
source/ifu/ifu_cache.sv
Outdated
| /////////////////// | ||
| // Internal Modules | ||
| /////////////////// | ||
|
|
||
| module updatePLruTree ( | ||
| module updatePLruTree |
There was a problem hiding this comment.
We don't have 2 modules in the same file.
Each file one module, and the name of the module and the file must match
source/ifu/ifu_cache.sv
Outdated
| if (freeLine == -1)begin | ||
| freeLine = getPLRUIndex(plruTree); | ||
| end | ||
| dataArray[freeLine] = mem_rspInsLineIn; |
There was a problem hiding this comment.
To avoid latches, give some default value at the beginning of the always block (let me know if I was clear). Additionally, if you use always_comb with inferred latches Quartus will give you an error
source/ifu/ifu_cache.sv
Outdated
| /////////////// | ||
| // Utilities // | ||
| /////////////// | ||
| task updatePLruTree(inout logic [NUM_LINES - 2 : 0 ] tree , input int line ); |
There was a problem hiding this comment.
please use a function instead or macros if possible. as Amichai posted. Tasks can be synthesizable if you write synthesizable code inside them. Task may work but lets not use them for proper coding style
verif/ifu/tb/ifu_cache_tb.sv
Outdated
There was a problem hiding this comment.
it's better to put all the parameters in the right package. If you use additional parameters for the test bench that are not in the package, then add them here.
There was a problem hiding this comment.
I see what you have been talking about. It's better to separate and put each module into different files.
verif/ifu/tb/PLRU_tb.sv
Outdated
There was a problem hiding this comment.
out simulator does not support assertion. you don't have to use assertion. If you want we have a special macro inside macros.vh that imitates that but its not necessary to use it
source/ifu/ifu.sv
Outdated
There was a problem hiding this comment.
It looks good the best way to evaluate it is to see it will work or not and how. good job
source/ifu/ifu_pkg.sv
Outdated
There was a problem hiding this comment.
its not necessary to declare parameter al logic. when you declare something as logic, it can be 0,1,x, Z. Its not wrong but for consistent coding style its better not to use logic
…tb name from PLRU to ifu_cache
change the the design of the cache,
using now always_comb blocks instead,
defined new interface with cpu,
interface with prefetecher should be added latre