Closed jrrk closed 4 years ago
@jonwoodruff issue opened for comment
I've done the proposed changes in this commit: https://github.com/ivanmgribeiro/ibex/commit/b2e82dd57913ed472d3da2da0d86de8df2d988f0
These allow the core to compile in Quartus 16.1 if the QUARTUS macro is defined in the project settings.
The currently used style (loop generate constructs, Chapter 27.4 of the SV spec) is recommended by our style guide at https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md#generate-constructs. The proposed alternative style (generate regions) is not recommended. So if we cannot make Quartus accept this syntax, we need to change the style guide. Ifdefs are really only the last resort.
To double-check if a style guide change is required:
I'm agreeing with @imphil re `ifdef. The proposed coding style in question should be supported, so let's pressure the tool vendors to explain why they are not conformant and fix as soon as possible. Worst case is we allow code exceptions and/or change the coding style in the short term. If we need to temporarily change the code to allow the tools to catch up, we should tag the change with a comment that allows us to find and remove when the tools are fixed.
On Tue, Jul 2, 2019 at 10:23 AM Philipp Wagner notifications@github.com wrote:
The currently used style (loop generate constructs, Chapter 27.4 of the SV spec) is recommended by our style guide at https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md#generate-constructs. The proposed alternative style (generate regions) is not recommended. So if we cannot make Quartus accept this syntax, we need to change the style guide. Ifdefs are really only the last resort.
To double-check if a style guide change is required:
- Can you retry with the latest Quartus version?
- Is this documented as missing feature in Quartus somewhere? According to https://www.intel.com/content/www/us/en/programmable/quartushelp/18.1/index.htm#hdl/vlog/vlog_list_sys_vlog.htm SV2009 should be supported (and loop generate constructs are SV2009). More details are hard to find in documentation, since unfortunately the documentation of language features ends at SV Spec Chapter 26, and generate blocks are discussed in Chapter 27 of the SV2009 spec.
- Did you check that Quartus doesn't need any special flag to choose the latest SystemVerilog version?
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/lowRISC/ibex/issues/117?email_source=notifications&email_token=AJZQKVLWWSWNCCRIHKXM5JLP5OFKBA5CNFSM4H44U5DKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZB7P6Y#issuecomment-507770875, or mute the thread https://github.com/notifications/unsubscribe-auth/AJZQKVIYA6NIBWABPTNH25TP5OFKBANCNFSM4H44U5DA .
I agree we should not unnecessarily clutter the code with ifdefs. But you will also appreciate that a large company like Intel will not prioritise the legacy Altera products. I had a good chat with Gary Guo who uses Altera and his conclusion was that the SysVerilog support in Quartus is pathetic. He recommends using Synplicity as a third-party solution, you will appreciate the very existence of this solution makes Quartus a non-priority. Since your coding style is signed off at a high level, and it is something people feel passionately about from a maintenance standpoint, I think we should we looking at a preprocessor to deal with obsolete Verilog-2000 tools and up and coming but not quite there tools like yosys.
Preprocessing to support tools that fall short of the the desired level of SystemVerilog support is definitely one approach. But we've already tried to limit ourselves to constructs we think are fairly widely supported - I think the next step on this issue is for someone to confirm whether the latest Quartus (I think 19.1) actually still struggles with this syntax, even when all necessary attempts have been taken to enable SystemVerilog support (extra command-line flags etc).
I haven't been able to try building with quartus 19.1 yet, but 18.1 still has the same issue.
Having the genvar x; outside of the for loop seems to conform to your coding style as follows:
genvar k;
for (k = 0; k < 32; k++) begin : gen_rev_operand_a
assign operand_a_rev[k] = operand_a_i[31-k];
end
Is this a change that could be implemented in the ibex source? If so, then when trying to use quartus to build it should be possible to simply add the generate and endgenerate lines in the local code or fork, which would mean that any changes made to the loop body in this repository should be easy to pull to the local code or other forks that are using quartus.
Is this a change that could be implemented in the ibex source? If so, then when trying to use quartus to build it should be possible to simply add the generate and endgenerate lines in the local code or fork, which would mean that any changes made to the loop body in this repository should be easy to pull to the local code or other forks that are using quartus.
Hi Ivan. That's definitely a possibility, but if we can tell people to use the latest Quartus instead then that's probably going to be the preferred option. If you can try 19.1 and exhaust all possible command line options for forcing maximal SystemVerilog support that would be really helpful. Let's determine definitively whether "use a newer Quartus" is a viable solution, and if not then lets look in more detail at potential workarounds.
Just to make sure I'm not misunderstanding: the code snippet you showed (genvar outside of the loop) does not build with Quartus as-is, but you still need to patch it to add generate/endgenerate statements?
Just to make sure I'm not misunderstanding: the code snippet you showed (genvar outside of the loop) does not build with Quartus as-is, but you still need to patch it to add generate/endgenerate statements?
Yes, that is correct. It does also work inside an always_comb block such as
always_comb
genvar k;
for (k = 0; k < 32; k++) begin : gen_rev_operand_a
operand_a_rev[k] = operand_a_i[31-k];
end
end
But I think that's also not allowed in your coding style, and it doesn't fix all the problems - there are still issues in the ibex_ex_block.sv file with the if (!MULT_TYPE) block.
So the next steps are:
Once we have exhausted all our options there, we can move on to either changing our style guide, or adding workarounds for Quartus.
Ivan asked me to comment on this. There are three versions of Quartus:
Quartus 18.1 SystemVerilog synthesis support says it supports SV 2005 and 2009, and links to the support table for Verilog (1995 and 2001). It claims that 'generated instantiation' is supported in Verilog 2001, but is unclear what that means.
It appears the SystemVerilog in Quartus Standard 18.1 is unhappy with the generate
statements. 19.1 Pro supports it. However Pro is essentially a substantial rewrite to target newer/larger FPGAs - I don't see Intel backporting any of these changes to Standard. In essence, to be able to target anything but the highest-end FPGAs, it would need code compatible with Quartus Standard's implementation.
Thanks for the clarification Theo!
Before moving further forwards on this, might either you or @ivanmgribeiro be able to confirm if there any other Quartus issues we should consider at the same time. Is this genvar issue the only current barrier to getting Ibex working on Quartus Standard 18.1?
Quartus Standard 18.1 also won't compile the following unless they are enclosed with generate and endgenerate: https://github.com/lowRISC/ibex/blob/6d09fb10608a144e5ec4bb1e37c4b16eca601ae0/rtl/ibex_ex_block.sv#L79-L85 https://github.com/lowRISC/ibex/blob/6d09fb10608a144e5ec4bb1e37c4b16eca601ae0/rtl/ibex_ex_block.sv#L115-L151
I'll leave @ivanmgribeiro to mention if there's any other issues, but just to clarify my above comment, I was confused about the different Quartus SV support in the documentation:
That means a baseline of SV2005 for most Intel FPGAs.
how about this:
`ifdef QUARTUS
`define GEN generate
`define ENDGEN endgenerate
`else
`define GEN
`define ENDGEN
`endif
...........................
`GEN
if (1) begin : crash
fubar inst(.clk, .rst, .in, .out);
end
`ENDGEN
Another thing which may be of note, although this doesn't seem to affect compilation in any way, is that https://github.com/lowRISC/ibex/blob/6d09fb10608a144e5ec4bb1e37c4b16eca601ae0/rtl/ibex_register_file_ff.sv#L19 gets brought up as a warning because the word "synthesis" is the first word in the comment, and per this help page, it seems quartus will try to read whatever comes next in the comment as a synthesis attribute, and it then throws a warning about the unrecognized synthesis attributes "or", "Verilator" and "simulation".
So the story here is:
To really decide if we should go for a style guide change, an exception/workaround in ibex, or for "it's simply not supported", we really need to answer the question "What would need to change in our style guide to go back to SV2005?" It's at least generate + localparam in ANSI port lists, but there might be more. Maybe we should have a design using all features in our style guide which can be synthesized and used to sanity-check the tool ...
@ivanmgribeiro would you be OK with applying local patches for now to get you unblocked, and I'm trying to find time to drive the style guide discussion in the next ~ 2 weeks?
Quartus Standard is maintained - new products like Cyclone 10 use it. Quartus Pro is a new rewrite targeting large FPGAs - I would expect Quartus Standard to remain the software for small/cheap FPGAs.
Synplify is not an option for those who don't have a Synopsys licence for it - for commercial purposes that's likely to be very expensive (I'd guess a 6+ figure sum), and for academic purposes it's only available if your institution is a member of a licensing club (for Europractice in the EU it's minimum EUR720 per year). While Quartus has some support for external tools, it is also not common to use Synplify for external synthesis (I've never seen a project using it).
A torture test sounds like a good idea. It would additionally help looking for regressions with new versions of the tools.
@tmarkettos "Quartus Standard is maintained": So you'd expect the parser to be updated to something more recent than SV2005?
So the story here is:
* Quartus Standard is a unmaintained product (like ISE was when Vivado came out)? * The academic and professional community can work around that by using Synplify for synthesis. * The hobbyist community typically cannot really work around it, with the Lite (== free as in beer) version being based on the Standard version.
To really decide if we should go for a style guide change, an exception/workaround in ibex, or for "it's simply not supported", we really need to answer the question "What would need to change in our style guide to go back to SV2005?" It's at least generate + localparam in ANSI port lists, but there might be more. Maybe we should have a design using all features in our style guide which can be synthesized and used to sanity-check the tool ...
@ivanmgribeiro would you be OK with applying local patches for now to get you unblocked, and I'm trying to find time to drive the style guide discussion in the next ~ 2 weeks?
Yes this is OK. Thanks for taking the time to do this.
@imphil @tmarkettos From what I have seen Quartus Standard is maintained in terms of minor library updates and bugfixes and not really synthesizer updates (here are release notes for the most recent version). I wouldn't expect Intel to improve on SV support on this Quartus variant any time soon.
I totally agree with keeping current coding style, as I consider required changes rather ugly.
What do you think about making a branch quartus
with required changes for generate
blocks after merging #336? We could add info in readme for all those wanting to test Ibex with Quartus.
I can provide patches and support in terms of updating and testing, so that the branch doesn't stale.
For reference I pushed required changes in quartus_generate branch.
It appears the SystemVerilog in Quartus Standard 18.1 is unhappy with the
generate
statements. 19.1 Pro supports it.
I can confirm that 19.1 Lite does not compile without explicit generate ... endgenerate
and genvar
.
Closing until things move on the Intel side. Summary of the discussion so far:
Quartus Lite and Standard are (apparently) not developed any further and don't have sufficient SystemVerilog support to compile Ibex. While we are willing to work around some tool deficiencies, the workarounds required for Quartus Lite/Standard contradict our style guide and we agreed that these workarounds are not acceptable in our code base. Quartus Lite/Standard can therefore not be used to synthesize Ibex. A full discussion of problems features is available in this issue, in PR #336, and at https://marekpikula.github.io/quartus-sv-gotchas/Intel%20Quartus%20SystemVerilog%20gotchas.html.
There are a couple workarounds:
You can also use this free tool: https://github.com/nbdd0121/sv-elaborator.git written by our own Gary Guo, which was developed to overcome a similar problem.
for (genvar ..) generate_statement() is not supported in Quartus
it likes
generate genvar g; for(g ...) generate_statement() endgenerate
The suggestion is made to add ifdef QUARTUS around the four places that you do this