EDA-2292/EDA-2296: Submodules Yosys-rs and Yosys-rs-plugin are uspdated

[ayyazahmed-rs]

No description provided.

[ayyazahmed-rs]

Hi @thierryBesson, here CI is failing for the testcases that are running on genesis technology(where memory_map() is used for BRAM mapping), this is because the changes that we made in memory_dff.cc are related to memory_libmap not w.r.t memory_map(). So, I look into it to separate the two flows with the help of a flag.

Regards,

[alain-rs]

@ayyazahmed-rs @awaisabbas-rs , this needs to be merged and pushed to Raptor.

[awaisabbas-rs]

Hi Alain,

There were some major changes applied to the BRAM inference to support new RS primitive inference for BRAM, There are some corner cases which CI catches and failed BRAM inference, we are fixing those issues, So,we will merge the PR asap once CI passes.

[thierryBesson]

@awaisabbas-rs @ayyazahmed-rs BTW I look deeper in your changes and you need modularization of your code e.g. create subfunctions taking care of the SDP/TDP cases in genesis3, not put all the code flat under "case GENESIS3'. So please create sub-functions and add comments also so it can be more readable. Thanks

[awaisabbas-rs]

Hi @thierryBesson, @alain-rs,

Is inference of new BRAM primitive TDP_RAM36K and TDP_RAM18x2K supported in VPR? as if it is not supported than we need to do so first so that it does not break the flow. Please let us know if it is already there than we can merge this PR as the CI is clean and create a PR at Raptor level.

[thierryBesson]

Hi @awaisabbas-rs this is always the basic question to ask and actually I guess so. Support should always start from the far end of the flow and progress up to the front end.
In case it is not supported yet what we can do is to add an option that disable the inference of the non yet supported primitives by default so that :

1. it does not break the flow : by default they are not inferred.
2. the back end team can still test by switching off the option so that primitives are then inferred and they can test and play with them.
3. When the whole flow works and has been validated, switch the default value of the option so that new primitives are always inferred. Eventually keep the option around for a while if we get issues with these new primitives so that we can disable their inference.

This would help to have a smooth transition process. Just an idea.

[alaindargelas]

@ManadherRS what is the status of the support, see question from @awaisabbas-rs above.

[alain-rs]

@awaisabbas-rs @thierryBesson correct me if I'm wrong, but looking quickly at the changes in this PR, it does not look like you can support both the old and the new mapping through an option. It looks like the old mapping is removed with this PR.

[thierryBesson]

@alain-rs I think we have too also because I asked @ayyazahmed-rs and @awaisabbas-rs to rewrite the code and modularize it. So we can do both : rewrite the code and add the option. So I see at the top level a trivial code like this:
if (option == OLD_BRAM_MAPPINg) {
performOldBramMapping();
} else {
performNewBramMapping();
}
Right now everything is flat everywhere and it is hard to read.

@ayyazahmed-rs please can you modularize the code, comment it and add this new option so that User can drive BRAM synthesis toward Old or New implementation ? By default we would call Old implementation till the other team mates are done.
Also on my side I'm stuck because I want to add the new synthesis flow and since we are touching the same file I cannot. So can we do this ASAP ?

[ayyazahmed-rs]

@thierryBesson , yes I am already working on it and added a new flag ( -new_tdp36k ) and based on it will run new bram mapping. I also need to separate these new changes by using scratchpad in memory_libmap. I will update you on it as soon as I completed and verify the flow.

Thanks,

[thierryBesson]

@ayyazahmed-rs ok perfect : so we agree that when this option is not invoked (which is the case by default) we use the former TDP inference as if nothing has changed ?