Aligned with HERO and solved 64-bits atomics warnings. #12
Conversation
yvantor
changed the title
There was a problem hiding this comment.
Thanks for your contribution, @yvantor! I have a few comments before we can merge this.
| end | ||
| end | ||
|
|
||
There was a problem hiding this comment.
Please avoid whitespace changes.
| initial begin | ||
| assert(AddrMemWidth+NumOutLog2 <= AddrWidth) else | ||
| $fatal(1,"Address not wide enough to accomodate the requested TCDM configuration."); | ||
| assert(NumOut >= NumIn) else | ||
| assert( (NumOut >= NumIn) | (Topology == tcdm_interconnect_pkg::LIC) ) else | ||
| $fatal(1,"NumOut < NumIn is not supported."); | ||
| end |
There was a problem hiding this comment.
Since those are static checks, could you move them out of the initial block? Then the synthesis tools cold also check the validity of those parameters.
if (AddrMemWidth+NumOutLog2 > AddrWidth)
$fatal(1,"Address not wide enough to accomodate the requested TCDM configuration.");
if ((NumOut < NumIn) && (Topology != tcdm_interconnect_pkg::LIC))
$fatal(1,"NumOut < NumIn is not supported.");There was a problem hiding this comment.
Ok! There are actually more assertions made the same way (such as this https://github.com/AlSaqr-platform/cluster_interconnect/blob/alsaqr_cluster/rtl/tcdm_interconnect/tcdm_interconnect.sv#L268 and also this https://github.com/AlSaqr-platform/cluster_interconnect/blob/alsaqr_cluster/rtl/tcdm_interconnect/tcdm_interconnect.sv#L302), should I apply the same modifications also there?
| @@ -191,7 +196,7 @@ module amo_shim #( | |||
| amo_result = swap_value_q; | |||
| // values are not euqal -> don't update | |||
| end else begin | |||
| amo_result = upper_word_q ? out_rdata_i[63:32] : out_rdata_i[31:0]; | |||
| amo_result = amo_res; | |||
There was a problem hiding this comment.
Do both implementations behave the same? In the one you changed, amo_result would get assigned to out_rdata_i[63:32] if both upper_word_q and DataWidth == 64. In your diff, it seems like you only take the DataWidth == 64 condition into account. Could you please check this?
There was a problem hiding this comment.
Thank you, it should be fixed now!
* Added basic manifest generation * top is optional in the manifest now * Added dependency on IP_LIB * Added functions to retrieve sources * Script updated and commented. * Added iverilog top module option and generic cli option argument. * cocotb iverilog cmake function. * Added cocotb library path. * cmds.f file generation. * closes pulp-platform#12 --------- Co-authored-by: Marco Andorno <[email protected]> Co-authored-by: Benoit Denkinger <[email protected]>
Changes:
Changes tested on HERO openmp-examples/tests-pulp/omp_atomic.