Skip to content

Conversation

Joonari
Copy link
Contributor

@Joonari Joonari commented Feb 13, 2025

This improves the Verilog support in function unit generator (FUGen). Also adds a register file generator (RFGen). Similarly to FUGen, it takes RF information from an ADF and generates corresponding RTL in either Verilog or VHDL.

@@ -683,8 +684,10 @@ FUGen::buildOperations() {
// Zero initialize this configuration to avoid simulation warnings
if (isLSU_ && minLatency_ < 3) {
Copy link
Contributor

@karihepola karihepola Feb 14, 2025

Choose a reason for hiding this comment

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

Are these redundant now? I suppose we must keep them if someone wants to force the '-' initialization. At least with Modelsim the 2 cycle LSU would have delta cycle warnings with don't care initializations, but should it be left for the user to decide?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. My understanding is that some synthesis tools can better optimize the design when you use don't care initializations, but I don't know how large effect this has nowadays. We could just leave '0' as the default value due to simulation behaviour. Then if the user wants to try optimizations, they can enable the don't care initialization.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these used in the testsuite? Before, these would print warnings with ghdl every cycle.

@@ -827,11 +848,12 @@ FUGen::buildOperations() {
} else {
operationCp.addVariable(SignedVariable(v.name, w));
}
// Zero initialize this configuration to avoid simulation warnings
if (isLSU_ && minLatency_ < 3) {
Copy link
Contributor

@karihepola karihepola Feb 14, 2025

Choose a reason for hiding this comment

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

Redundant?

@@ -841,12 +863,15 @@ FUGen::buildOperations() {
// Zero initialize this configuration to avoid simulation warnings
if (isLSU_ && minLatency_ < 3) {
Copy link
Contributor

@karihepola karihepola Feb 14, 2025

Choose a reason for hiding this comment

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

Redundant?

Copy link
Contributor

@pjaaskel pjaaskel left a comment

Choose a reason for hiding this comment

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

Mostly superficial comments from a quick glimpse. Good useful stuff. Quite many commits, next time perhaps check if you can squash some of the commits before pushing to make the history more easily readable/reviewable.

}

/**
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra space before and after.

@@ -0,0 +1,86 @@
/*
Copyright (C) 2024 Tampere University.
Copy link
Contributor

Choose a reason for hiding this comment

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

-2025?

/*
Copyright (C) 2024 Tampere University.

This library is free software; you can redistribute it and/or
Copy link
Contributor

Choose a reason for hiding this comment

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

We use LGPL v2.1 for the toolset files.


namespace ProGe {

class GeneratableRFNetlistBlock : public NetlistBlock {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not indent because of the namespace.

@@ -622,22 +632,38 @@ ProGeUI::generateIDF(
}
}

// RFs.
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove the commented out code or at least comment it out with #if 0?

@@ -1503,6 +1503,9 @@ namespace HDLGenerator {
<< ident << "use ieee.std_logic_1164.all;\n"
<< ident << "use ieee.numeric_std.all;\n"
<< ident << "use ieee.std_logic_misc.all;\n"
<< ident << "use STD.textio.all;\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

ident? indent?

@@ -209,6 +254,77 @@ RFGen::createRFReadProcess() {
behaviour_ << RawCodeLine(vhdlCode, verilogCode);
}

/*
* Create process to dump RF values into a file during simulation.
* Useful for debugging RISC-V machines, so create this
Copy link
Contributor

Choose a reason for hiding this comment

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

Add empty line after the brief comment.


BoolCmdLineOptionParser* dontCareInit = new BoolCmdLineOptionParser(
DONT_CARE_INIT,
"Initialize FUGen generated signals as don't care. E.g. some FPGA tool optimizations prefer these.");
Copy link
Contributor

Choose a reason for hiding this comment

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

long line

@@ -102,15 +102,11 @@ TestBenchBlock::~TestBenchBlock() {
void
TestBenchBlock::write(const Path& targetBaseDir, HDL targetLang) const {
// Check language compatibility //
std::cout << "--1" << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you squash this with the commit that introduced the debug prints. Now there's 77 commits which is quite a lot? Fixup commits help to clean up the history.

@@ -1,210 +0,0 @@
#! /bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you just revert the commit where you added it or just squash this to it?

Joonari added 26 commits March 14, 2025 16:53
Can generate netlist block and RF ports now.
Signal formatting was missing a bracket when sign extending, and
signal load_data_32b was not declared
Add --dont-care-init option to generateprocessor to initialize signals as ('-').
Don't care values cause issues in ASIC synthesis tools, so use '0'
as the default init value.
Change order of verilog include files containing parameter declarations,
and add IMEMDATAWIDTH parameter to verilog package file
Module's 'd' port was initialized with zeroes. Verilog replication operator does not allow
using a parameter of the same block to do this.
@Joonari Joonari force-pushed the fugen-verilog-fixes branch from 965ca7a to c581faa Compare March 14, 2025 14:55
@Joonari
Copy link
Contributor Author

Joonari commented Mar 14, 2025

Fixed the style issues pointed out by @pjaaskel and squashed some commits to clean up the log.

@pjaaskel
Copy link
Contributor

Is this ready?

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.

3 participants