-
Notifications
You must be signed in to change notification settings - Fork 13
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
Fasm annotation support #32
base: main
Are you sure you want to change the base?
Conversation
Hi @mkurc-ant, I think we should have a quick chat about this PR. I think we do want to add fasm support but I would like the fasm features to be actually connected to the verilog names rather than having no relationship to them. The reason I want this is that v2x is suppose to describe the hardware. The FASM features describe configuration of the hardware. Hence there should be a pretty direct mapping between the two. The goal here would be to be able to tag various things with a generic Have you been doing this for the QuickLogic support? If so, it might be a good idea for me to also look at some real world examples to make it easier to make sure I'm not missing something. Tim '@mithro' Ansell |
@mithro I agree that it would be best to have a one-to-one correspondence between verilog model names and fasm features. But I'm not sure whether that would be possible in all cases. Sometimes the architecture for the VPR need to differ from the physical underlying hardware simple due to the way that the VPR works. Often structure for fasm features in a database also does not exactly reflect the hardware structure. That's why I'd opt for giving a user some freedom for fasm annotation. And yes, I've implemented the fasm annotation with the QuickLogic support in mind as a first real-world use case of the V2X. You can look how the annotation is done here: https://github.com/antmicro/symbiflow-arch-defs/tree/master%2Bquicklogic/quicklogic/primitives. The annotation is present for LOGIC and BIDIR primitives. Fasm feature names are generated from the vendor database, they are stored in the One think I'm not happy about is specification of fasm parameters. Since there is likely be no support for attributes on parameters in the upstream Yosys, parameters cannot be easily annotated. Hence the way of specifying fasm_params i rather tedious and I'm not particularly happy with it as it is done in this PR. But I couldn't think of any better solution.
|
If the task is to fish out attributes from (System)Verilog, maybe using the Python-API from Surelog might help ? https://github.com/alainmarcel/Surelog#python-api With a strategic listener for
|
@hzeller It's not that simple. In V2X we use Yosys to convert Verilog modules to JSON representation which is then processed by a Python script. We could use another tool to additionally extract attributes bu then it would create additional dependency for VTX. Not sure if we want that. |
@mkurc-ant - Can you give @hzeller's idea a go? |
@mithro Ok, I'll try to integrate that. So I imagine that it could work in this way: A user specifies eg. the Example:
And that would be translated into the following metadata entry:
Does that make sense? |
|
||
generate if (MODE == "MODE_A") begin | ||
assign O = I0; | ||
end else if (MODE == "MODE_B") begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want a FASM_FEATURES_MODE_B
system. I think this can be done by just using the FASM features inside the generate statement below? IE (* FASM_FEATURES="SEL_MODE_B;ENABLE_SOMETHING" *)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be done in that way up to some extent. Imagine that you have a model that has two modes of which one is "passthrough" (no bels inside). Then there would be nothing to attach these features to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better for you to provide concrete examples as tests, that way there is no confusion about what is being discussed.
Please add the example you are concerned about as a test and we can discuss it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkurc-ant - You haven't responded to this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkurc-ant Can you assign the fasm feature to the assign statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkurc-ant - ping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mithro I've checked two alternative options:
- Attach the (* FASM_FEATURES *) attribute to the assign statement. Unfortunately Yosys does not support that - it crashes with a Verilog syntax error message.
- Attach an attribute to the generate statement. This time Surelog reported a Verilog syntax error.
An example of a situation with a "passthrough" mode where a fasm feature needs to be emitted is the ILOGIC site in XC7. Basically to model that using V2X we would exactly need to support attributes on assign statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mithro Any thoughts on that?
tests/fasm_params/module.sim.v
Outdated
input wire I1; | ||
input wire I2; | ||
output wire O; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done by just applying the (* FASM *)
attribute to the parameter.
(* FASM *)
parameter INIT = 0;
I think we should also require the parameter name to match the fasm feature name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really in favor of enforcing the name match. For example in XC7 LUTs we have something like that (this is ntemplateNlut.pb_type.xml
:
<metadata>
<meta name="fasm_type">SPLIT_LUT</meta>
<meta name="fasm_lut">
{N}LUT.INIT[31:0] = {N}5LUT[0]
{N}LUT.INIT[63:32] = {N}5LUT[1]
</meta>
</metadata>
We could make the assumption of equal names a default but do not enforce it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue I have with letting people override the name is that people will use the renaming heavily rather than fixing either the fasm or the verilog file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkurc-ant - ping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. So I've fixed that. Now only the (* FASM *)
attribute is supported and it binds a parameter with a fasm feature of the same name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the change that requires parameter names to be equal to fasm feature names. But this won't work eg. for existing Quicklogic support. For example the BIDIR
cell there uses fasm features that (without prefix) are named like: "INV.ESEL" etc. A verilog parameter cannot have a dot in its name. https://github.com/QuickLogic-Corp/symbiflow-arch-defs/blob/3393d6b32cbb3ecc779b7c0a953fd6963987e1a2/quicklogic/pp3/primitives/bidir/bidir_cell.sim.v#L3
tests/fasm_prefix/parent.sim.v
Outdated
// assigned a different fasm prefix. | ||
genvar i; | ||
generate for (i=0; i<2; i=i+1) begin | ||
(* FASM_PREFIX = "PREFIX_FOR_GATE_X;PREFIX_FOR_GATE_Y" *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep this syntax, the names would be probably be better called (* FASM_PREFIX = "PREFIX_FOR_GATE_I0;PREFIX_FOR_GATE_I1" *)
.
I wonder if the syntax should be something like (* FASM_PREFIX = "[PREFIX_FOR_GATE_I0, PREFIX_FOR_GATE_I1][i]" *)
?
I'm wondering if the array should be associated on the genvar
or generate
statement not the gate itself? The
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So here we meet another Yosys limitation: it discards an attribute specified on a genvar and fails to parse an attribute on a generate statement (unclear whether the latter is allowed in Verilog).
Could you elaborate more on the syntax with array you proposed? Should the [i]
be treated as an expression in V2X so one could do eg. [2*i+1]
?
I was also thinking of using a formatting syntax (like eg. in printf). But that won't allow to have a generate for cells with prefixes like ALUT
, BLUT
, CLUT
and so on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkurc-ant - ping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately Yosys does not allow to assign attributes on generate statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mithro ping?
- `(* FASM_PREFIX="prefix" *)` : Sets the fasm prefix of a cell instance. | ||
Cannot be set on a module definition! | ||
|
||
- `(* FASM_PREFIX="prefix1;prefix2;..." *) : Same as FASM_PREFIX but used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm less happy with this extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I have no better idea how to deal with this use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkurc-ant - ping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mithro How would you suggest to set fasm prefix on cell instances created using a generate
statement? I couldn't think of anything better than (* FASM_PREFIX="prefix1;prefix2;..." *)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mithro ping?
for the given module. Has to be provided at a module definition. | ||
Multiple features have to be separated by semicolon. | ||
|
||
- `*( FASM_FEATURES_{mode}="str1;str2;..." *)` : Same as above but defines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to rework how this one works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the use case here is that we need to emit a feature only in specific mode that is not directly related to any of child cells.
For example: we have a module that holds either 1xSRL32 or 2xSRL16 - these are different modes. When using 2xSRL16 we need to emit the SMALL
feature. We could specify it in the feature list for both child SRL16s but instead we can bind it to the mode itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkurc-ant - ping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another example from XC7: There is the ILOGICE3 site which can host ISERDES, IDDR or nothing for a passthrough mode. In the passthrough mode there has to be a fasm feature emitted which configures one of muxes.
So the site is modeled as a pb_type with 3 modes: PASSTHROUGH, ISEDRES_NO_IDELAY, ISERDES_IDELAY and IDDR (will be). In the PASSTHROUGH mode there are no leaf pb_types, just an interconnect. The required feature is attached to one of its connections. See https://github.com/SymbiFlow/symbiflow-arch-defs/blob/9d0b8f25beec7073345a232b7e8512be6d5e8652/xc/common/primitives/ilogice3/ilogice3.pb_type.xml#L33
Ideally the fasm feature should be attached to an assign statement modeling the interconnect. But unfortunately Yosys does not support that.
The problem is not relevant eg. in the QuickLogic architecture but it will hit us eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mithro Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should only add the feature when an example like that is added as a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a bunch of comments around the FASM annotations.
This is required to me merged in order for the Surelog integration to work: https://github.com/alainmarcel/Surelog/pull/337. Also a new conda Surelog package with that fix has to be available. |
(I think that Travis wasn't triggered for the latest push...) |
@mkurc-ant - Can you create a longer term issue to look into if using Surelog rather than Yosys is a better solution for |
@mithro Could you take look at this PR again? I think I addressed all the issues. Right now it is a hybrid that uses Yosys to parse & elaborate verilog descriptions and Surelog just to get attributes from module parameters (for fasm annotation). |
@mithro can you take a look on this again? This is required for QuickLogic support |
@kgugala - I'm still waiting for @mkurc-ant to reply on all the unresolved issues? The biggest blocking one is #32 (review)
But there is also multiple others. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkurc-ant -- What is the status here?
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
…o create the "fasm_params" metadata section. Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Alessandro Comodi <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
ef0ae25
to
6df93e2
Compare
@mithro I've updated documentation for fasm annotation support. Please review. I also want to point out an issue with enforcing equal names of FASM features and Verilog parameter names. For example in U-Ray database flip-flop initialization features are named I suggest that we re-evaluate the approach when you can either specify the @acomodi FYI |
Why not just create a module block called |
@mithro So the whole hierarchical name of the feature is But there are more features for FFs with different prefixes:
And here we cannot pack the FF primitive into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still worried about the fasm_prefix
addition. Everything else looks good.
(* FASM_FEATURES_MODE_A="SEL_MODE_A;ENABLE_SOMETHING_A" *) | ||
(* FASM_FEATURES_MODE_B="SEL_MODE_B;ENABLE_SOMETHING_B" *) | ||
(* MODES="MODE_A;MODE_B;MODE_C" *) | ||
module MODULE(I0, I1, O); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do any of these work?
Attribute on the begin block
(* FASM_FEATURES="IN_USE" *)
(* MODES="MODE_A;MODE_B;MODE_C" *)
module MODULE(I0, I1, O);
parameter MODE = "MODE_A";
input wire I0;
input wire I1;
output wire O;
generate if (MODE == "MODE_A") (* FASM_FEATURES="SEL_MODE_A;ENABLE_SOMETHING_A" *) begin
GATE gate (I1, I0, O);
end else if (MODE == "MODE_B") (* FASM_FEATURES="SEL_MODE_B;ENABLE_SOMETHING_B" *) begin
GATE gate (I0, I1, O);
end else if (MODE == "MODE_C") begin
assign O = I0;
end endgenerate
Attribute on a dummy wire
(* FASM_FEATURES="IN_USE" *)
(* MODES="MODE_A;MODE_B;MODE_C" *)
module MODULE(I0, I1, O);
parameter MODE = "MODE_A";
input wire I0;
input wire I1;
output wire O;
generate if (MODE == "MODE_A") begin
(* FASM_FEATURES="SEL_MODE_A;ENABLE_SOMETHING_A" *)
wire GO;
GATE gate (I1, I0, GO);
assign O = GO;
end else if (MODE == "MODE_B") begin
(* FASM_FEATURES="SEL_MODE_A;ENABLE_SOMETHING_B" *)
wire GO;
GATE gate (I1, I0, GO);
assign O = GO;
end else if (MODE == "MODE_C") begin
assign O = I0;
end endgenerate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Attribute on the begin block" - Unfortunately Yosys does not accept attributes neither on a begin ... end
block nor on generate
statement (syntax error).
"Attribute on a dummy wire" - This works. I think we could utilize that approach. Maybe we could collect all FASM_FEATURES
attributes from all wires of a module? This would allow to assign them to non-dummy wires (if any) as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mithro ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try the attributes on the wire approach?
(* FASM_PREFIX = "FASM_PREFIX_FOR_GATE_A" *) | ||
GATE gate_a (I0, I1, O0); | ||
(* FASM_PREFIX = "FASM_PREFIX_FOR_GATE_B" *) | ||
GATE gate_b (I0, I1, O1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this not be;
// Explicit instances of multiple children of the same type.
(* FASM *)
GATE fasm_prefix_for_gate_a (I0, I1, O0);
(* FASM *)
GATE fasm_prefix_for_gate_b (I0, I1, O1);
IE Why are the fasm and object names different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want to use the cell name as FASM prefix? In my opinion that would be too restrictive (as it is with FASM parameters).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mithro ping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being restrictive is the point. :-)
Making sure people keep consistent between the FASM and the Verilog is going to be hard without some type of forcing function.
// assigned a different fasm prefix. | ||
genvar i; | ||
generate for (i=0; i<2; i=i+1) begin | ||
(* FASM_PREFIX = "PREFIX_FOR_GATE_I0;PREFIX_FOR_GATE_I1" *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inside a generate statement like this could be a good case where fasm_prefix is actually required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be required whenever you want to model a piece of hardware that has multiple identical elements. That could be eg. LUTs and FFs of Xilinx 7-series SLICE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mithro ping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we just make people explicitly create the elements and prohibit generate statements?
for the given module. Has to be provided at a module definition. | ||
Multiple features have to be separated by semicolon. | ||
|
||
- `*( FASM_FEATURES_{mode}="str1;str2;..." *)` : Same as above but defines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should only add the feature when an example like that is added as a test.
1c1c2c3
to
6df93e2
Compare
@mkurc-ant - What is needed to finish this off? |
This PR adds a preliminary support for adding fasm annotations.
Supported constructs are:
Documentation how to specify those is added to the
vlog_to_pbtype.py
header and is also present in README files for tests.Due to lack support of attributes on parameters in Yosys (they are parsed but discarded and do not end up in a JSON file) support for fasm params is a bit tedious. I'm open to suggestions how to do it in a better way.