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

ice40/ecp5: add support for both 1364.1 and Synplify/LSE RAM/ROM attributes #1603

Merged
merged 8 commits into from Apr 10, 2020

Conversation

whitequark
Copy link
Member

@whitequark whitequark commented Jan 1, 2020

This commit tries to carefully follow the documented behavior of LSE
and Synplify. It will use syn_ramstyle attribute if there are any
write ports, and syn_romstyle attribute otherwise.

  • LSE supports both syn_ramstyle and syn_romstyle.
  • Synplify only supports syn_ramstyle, with same values as LSE.
  • Synplify also supports syn_rw_conflict_logic, which is not
    documented as supported for LSE.

Limitations of the Yosys implementation:

  • LSE/Synplify appear to interpret attribute values insensitive
    to case. There is currently no way to do this in Yosys (attrmap
    can only change case of attribute names).
  • LSE/Synplify support syn_ramstyle="block_ram,no_rw_check"
    syntax to turn off insertion of transparency logic. There is
    currently no way to support multiple valued attributes in
    memory_bram. It is also not clear if that is a good idea, since
    it can cause sim/synth mismatches.
  • LSE/Synplify/1364.1 support block ROM inference from full case
    statements. Yosys does not currently perform this transformation.
  • LSE/Synplify propagate syn_ramstyle/syn_romstyle attributes
    from the module to the inner memories. There is currently no way
    to do this in Yosys (attrmvcp only works on cells and wires).

@whitequark
Copy link
Member Author

  • LSE/Synplify appear to interpret attribute values insensitive
    to case.

I've added some code to memory_bram to handle this case as well.

@whitequark whitequark changed the title ice40: add support for both 1364.1 and LSE RAM/ROM attributes ice40/ecp5: add support for both 1364.1 and LSE RAM/ROM attributes Jan 1, 2020
@whitequark
Copy link
Member Author

I did a similar update of ECP5 BRAM rules, taking into account LUTRAM.

techlibs/ecp5/bram.txt Outdated Show resolved Hide resolved
@whitequark whitequark force-pushed the ice40-ram_style branch 2 times, most recently from ffe8cbd to 693b267 Compare January 1, 2020 10:13
@whitequark
Copy link
Member Author

whitequark commented Jan 1, 2020

I've also adjusted memory_map so that it can avoid mapping memories that are explicitly required to use not FFRAM (i.e. BRAM or, if available, LUTRAM). This fixes the longstanding Yosys issue where if you make a mistake describing a BRAM behaviorally, it goes off to spend ten minutes mapping a megabit of RAM to DFFs.

@whitequark whitequark force-pushed the ice40-ram_style branch 2 times, most recently from b8b3d75 to a9be33d Compare January 1, 2020 12:43
@whitequark
Copy link
Member Author

whitequark commented Jan 1, 2020

Thinking out loud about possible improvements (I have no immediate plans to implement any of this):

  • It seems valuable to have a single pass that would case-normalize Synposys attributes and where applicable map them to Yosys ones (syn_keepkeep seems particularly important). You could go quite far with just attrmap, but it seems ugly to duplicate it every time a vendor toolchain uses Synopsys-compatible attributes, since most of them do.
  • Multiple valued attributes should probably be supported similarly to "strpool" attributes, except that they are split by ,, the accessors operate on a vector<string> in addition to pool<string>, and whitespace is stripped.
  • Extending attrmvcp to handle modules seems valuable because there are quite a few Synopsys attributes that need it (syn_use_carry_chain, syn_useenables, ...) but also there are Lattice-specific ones (GSR), so just doing this in Synopsys-specific code is probably not enough.
  • Existing memory_bram logic should be able to handle no_rw_check just fine, so whoever thinks that this is a good idea (I am unconvinced) can probably implement it easily once multiple valued attributes are in.

@whitequark whitequark changed the title ice40/ecp5: add support for both 1364.1 and LSE RAM/ROM attributes ice40/ecp5: add support for both 1364.1 and Synplify/LSE RAM/ROM attributes Jan 1, 2020
@eddiehung
Copy link
Collaborator

Another related thing to just mention for discussion is that Synopsys directives can also appear as comments, so if there are improvements ahead it may be worth planning for that too...

@whitequark
Copy link
Member Author

Another related thing to just mention for discussion is that Synopsys directives can also appear as comments, so if there are improvements ahead it may be worth planning for that too...

Let me elaborate a bit on my strategy here. When adding attributes like this one, it is good practice to do the least surprising thing for the HDL designer. In this case, to me, this means not inventing new attributes or meanings if at all possible; and so I am reusing Synopsys attribute names and values (even though they are inconsistent and a bit clunky), as well as 1364.1 ones (which I suspect were influenced by Synopsys).

Separately from that there is the issue of hot comments. I think they are an abomination, and Yosys certainly does not disagree, as the warnings suggest. I have actually considered implementing them and spent a bit of time before deciding that some things are best left in the past. It's been almost a decade since Verilog-2001, isn't it finally time to start using it? (Yes, you can't really do // synthesis translate off with attributes, but here we are discussing syntax that is exactly as powerful as Verilog-2001 attributes, except worse in every other aspect.)

@eddiehung
Copy link
Collaborator

Separately from that there is the issue of hot comments. I think they are an abomination, and Yosys certainly does not disagree, as the warnings suggest. I have actually considered implementing them and spent a bit of time before deciding that some things are best left in the past. It's been almost a decade since Verilog-2001, isn't it finally time to start using it? (Yes, you can't really do // synthesis translate off with attributes, but here we are discussing syntax that is exactly as powerful as Verilog-2001 attributes, except worse in every other aspect.)

I'm not advocating their continued use, but it may be an unavoidable necessity for supporting legacy code. One solution may be to convert those hot comments into attributes.

@daveshah1
Copy link

Perhaps comments of this format should produce a warning advising the correct attribute but not actually be honoured, to push people away from them.

@whitequark
Copy link
Member Author

One solution may be to convert those hot comments into attributes.

If Yosys were to support such hot comments, I think converting them into attributes is the only realistic option. But I really like @daveshah1's suggestion of parsing them without honoring them. If there is no strong objection against it I might implement it; it's certainly true that an unfortunate amount of documentation, even recent, suggests using hot comments.

it may be an unavoidable necessity for supporting legacy code

That is true. In my view, the best way to handle hot comments in legacy code is to release a tool that automatically converts them to Verilog-2001 attributes. Perhaps one already exists? If not, then one of the recently developed Verilog parsers may be useful (in terms of providing precise source locations) for such a tool. I wrote a few tools like that and could certainly do another.

@whitequark
Copy link
Member Author

Another option would be to (re)use Yosys' lexer to make a much more hacky code transformation tool tailored specifically to this issue. Without relying on a tooling-grade parser it is harder to make one that is 100% reliable because you can't easily check if the transformation you just done is indeed correct, but it might still be good enough for most uses.

iCE40 does not have LUTRAM. This was erroneously added in commit
caab661, and tested for BRAM,
essentially a duplicate of the "dpram.ys" test.
This commit tries to carefully follow the documented behavior of LSE
and Synplify. It will use `syn_ramstyle` attribute if there are any
write ports, and `syn_romstyle` attribute otherwise.
  * LSE supports both `syn_ramstyle` and `syn_romstyle`.
  * Synplify only supports `syn_ramstyle`, with same values as LSE.
  * Synplify also supports `syn_rw_conflict_logic`, which is not
    documented as supported for LSE.

Limitations of the Yosys implementation:
  * LSE/Synplify appear to interpret attribute values insensitive
    to case. There is currently no way to do this in Yosys (attrmap
    can only change case of attribute names).
  * LSE/Synplify support `syn_ramstyle="block_ram,no_rw_check"`
    syntax to turn off insertion of transparency logic. There is
    currently no way to support multiple valued attributes in
    memory_bram. It is also not clear if that is a good idea, since
    it can cause sim/synth mismatches.
  * LSE/Synplify/1364.1 support block ROM inference from full case
    statements. Yosys does not currently perform this transformation.
  * LSE/Synplify propagate `syn_ramstyle`/`syn_romstyle` attributes
    from the module to the inner memories. There is currently no way
    to do this in Yosys (attrmvcp only works on cells and wires).
Some vendor toolchains use case insensitive matching for values of
attributes that control BRAM inference.
LSE/Synplify use case insensitive matching.
@whitequark
Copy link
Member Author

Rebased the PR on top of master. Could I get a review please?

cc @eddiehung @cliffordwolf since this changes memory_map and memory_bram
cc @daveshah1 since this changes iCE40/ECP5 brams.txt
cc @mwkmwkmwk since this does not change Xilinx brams.txt; I could probably do the change but I don't use those devices myself and lack the context right now
cc @ZirconiumX since this does not change Intel brams.txt and I'm not even sure if it recognizes these attributes

anyone I forgot? Achronix, Anlogic, Efinix, Gowin? I'm not sure if those targets care about BRAM attributes yet.

@Ravenslofty
Copy link
Collaborator

Ravenslofty commented Feb 6, 2020

Quartus does recognise syn_ramstyle but interprets its contents a little differently.

EDIT: That's actually going to cause a notable headache for me, hmmm.

This commit tries to carefully follow the documented behavior of LSE
and Synplify. It will use `syn_ramstyle` attribute if there are any
write ports, and `syn_romstyle` attribute otherwise.
  * LSE supports both `syn_ramstyle` and `syn_romstyle`.
  * Synplify only supports `syn_ramstyle`, with same values as LSE.
  * Synplify also supports `syn_rw_conflict_logic`, which is not
    documented as supported for LSE.

Limitations of the Yosys implementation:
  * LSE/Synplify support `syn_ramstyle="block_ram,no_rw_check"`
    syntax to turn off insertion of transparency logic. There is
    currently no way to support multiple valued attributes in
    memory_bram. It is also not clear if that is a good idea, since
    it can cause sim/synth mismatches.
  * LSE/Synplify/1364.1 support block ROM inference from full case
    statements. Yosys does not currently perform this transformation.
  * LSE/Synplify propagate `syn_ramstyle`/`syn_romstyle` attributes
    from the module to the inner memories. There is currently no way
    to do this in Yosys (attrmvcp only works on cells and wires).
Copy link
Collaborator

@cliffordwolf cliffordwolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the changes to memory_bram and memory_map. For the most part they LGTM. See comments for two minor requested changes.

passes/memory/memory_bram.cc Outdated Show resolved Hide resolved
passes/memory/memory_map.cc Show resolved Hide resolved
Before this commit, memory_map (which is always a part of a synth
script) would always pick up any $mem cell that was not processed
by a preceding pass and lower it down to $dff/$mux cells.
This is undesirable for two reasons:
  * If there is an explicit inference attribute set on a $mem cell,
    e.g. (* ram_block *), then it is arguably incorrect to map such
    a memory to $dff/$mux cells.
  * If memory_map tries to lower a memory that was intended to
    be mapped to a large BRAM, it often takes extraordinarily long
    time to finish, produces an extremely large log file, and outputs
    an unusable design.

After this commit, properly invoked memory_map will not map any
memory that has an explicit inference attribute specified, solving
the first issue, and alleviating the second. The default behavior
is not changed.
@nakengelhardt
Copy link
Member

@whitequark Sorry, I didn't quite manage to follow the status of this PR. Can you give me a bit of context? Do you want Claire to look at the whole thing or just the changes to memory_map/memory_bram? Is this ready to merge otherwise?

@whitequark
Copy link
Member Author

whitequark commented Apr 7, 2020

@nakengelhardt I fixed the two requested minor changes, hence the review request. There were no other changes to the PR. It is totally ready to merge otherwise.

@whitequark
Copy link
Member Author

Other than that, @ZirconiumX is already aware of the changes (which is all I wanted here), and @mwkmwkmwk never responded here but there is no particular action needed from her, I just wanted to give a heads up about implementing Synplify attributes so that other targets could take it into account.

@whitequark whitequark merged commit 93ef516 into YosysHQ:master Apr 10, 2020
@whitequark whitequark deleted the ice40-ram_style branch April 10, 2020 14:51
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

Successfully merging this pull request may close these issues.

None yet

6 participants