Reorganizing vqm2blif source code #1850

Srivat97 · 2021-09-15T23:19:15Z

Re-factored the original vqm2blif program source code for improved modularity. To verify the modifications, a test script was also added to the codebase.

Description

Major Change 1:

cleanup.cpp contained a function called "remove_node", this function was written in a way that only allowed the "remove_one_lut_nodes" function to call it. The change allowed other functions to be able to directly use the "remove_node" function.
also in cleanup.cpp, the function "remove_one_lut_nodes" had an embedded set of operations that basically fixed an array of nodes with gaps inside (empty elements), such that the gaps were removed and the array was continuous. The change added a separate function called "reorganize_module_node_list", which accepted a node array with gaps (empty elements) inside and returned an array with the gaps removed.

Major Change 2:

Added a test script called "test_vqm2blif.sh". This script can be used to test the vqm2blif program with any arbitrary netlist and architecture file. This script will inform the user if the program failed or whether the generated blif netlist was incorrect.
Also added 3 basic benchmark netlists that can be used to verify the vqm2blif program.

Major Change 3:

Modified the github workflow file (test.yml) and added an additional job to test to verify the vqm2blif program. This new job leveraged the newly created test script described in change 2 above.

Related Issue

Motivation and Context

The code re-factoring was was done to simply improve the modularity of the codebase.
There was a lack of verification for the vqm2blif program, so the test script and workflow modifications were done to allow the vqm2blif program to be verified.

How Has This Been Tested?

The vqm2blif program test script was tested first with the original unmodified vqm2blif source code. This helped verify whether the script can correctly determine when the program works successfully.
For the second test, changes were made such that the vqm2blif program would fail. The test script was executed and this helped confirm whether the script successfully notified the user when the vqm2blif program failed during conversion.
For the the third test, changes were made to the vqm2blif program such that the generated blif netlists would be incorrect and this helped confirm whether the script successfully notified the user when the generated blif netlists did not match the "golden" results; thus letting the user know that their changes incorrectly modified the program.

The re-factored changes to the source code were verified using the test script above on the titan benchmarks.

To ensure that the changes did not affect other parts of the source code, the VTR reg tests were all run.

Types of changes

Bug fix (change which fixes an issue)
New feature (change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation
I have updated the documentation accordingly
I have added tests to cover my changes
All new and existing tests passed

…any type of nodes

…ing nodes

…lso updated prototypes to match existing convention

…vqm2blif program

… replaced a string literal with its variable representation.

…blif program.

vaughnbetz · 2021-09-17T16:18:55Z

There was a merge conflict in test.yml that was stopping kokoro from running; resolved it by adding both new lines (Seyed's and Srivatsan's).

vaughnbetz · 2021-09-17T17:04:42Z

utils/vqm2blif/src/base/cleanup.cpp

+		}
+	}
+	if (module_node_list[curr_node_index] == NULL) {
+		VTR_ASSERT(curr_node_index == new_array_size); //check array size


// check array size is too low level a comment (doesn't really tell me what you're doing except that it is a check, which I can already tell from the assert.
Instead, comment the whole if statement to explain what you're checking, and why you need two cases.

This entire block has now been removed. Originally, this assert block was used to verify whether the node list had the correct number of nodes after we had filled in all the gaps. The user originally supplied a parameter of the number of nodes within the array that were removed, using this a new array size was determined and this was compared to size of the node array after filling in all the gaps. But now, since we do not pass in the number of nodes removed, there is no need to perform this check as we now internally determine the node array size as we fix it.

vaughnbetz · 2021-09-17T17:08:34Z

utils/vqm2blif/src/base/cleanup.cpp

+	/*
+        The purpose of this function is to regorganize the nodes array by filling in gaps with the last available elements in the array to save CPU time.
+
+		The node array provided to this function is expected to have elements deleted within. THis essentially creates gaps in the array. So we fill in the gaps of this array and ensure it is a continuous list of nodes.


explain that the node array is module_node_list, and that deleted elements within it are NULL.
Do you need original_number_of_nodes, or would that be equal to module->number_of_nodes?
number_of_nodes_eliminated seems dangerous to pass in. It should be redundant with the number of NULL entries in the module_node_list, so it doesn't have to be passed. It isn't asserted to match the number of NULLs eliminated, so it is also a bit dangerous -- if it doesn't match then the module_node_list could be corrupted.

This has been updated now. number_of_nodes and nodes have been removed. Additionally, the number_of_nodes_eliminated has also been removed as a parameter and we are now determining this within the function.

the function "remove_one_lut_nodes" also had unnecessary parameters that could have been found through "module", so the function was also changed as well.

vaughnbetz · 2021-09-17T17:09:24Z

utils/vqm2blif/src/base/cleanup.h

@@ -19,6 +19,10 @@ extern int buffers_elim, inverts_elim, oneluts_elim;

 void netlist_cleanup (t_module* module);

+void remove_node ( t_node* node, t_node** nodes, int original_num_nodes );
+
+void reorganize_module_node_list(int original_number_of_nodes, int number_of_nodes_eliminated, t_node** module_node_list, t_module* module);


Comment what these functions do here (in the header) so people calling them just have to read the header. The comment should explain what each input to the function is, and what the function does (but doesn't have to explain how the function does it).

Additional comments have been added now.

vaughnbetz · 2021-09-17T17:18:55Z

utils/vqm2blif/test/scripts/test_vqm2blif.sh

@@ -0,0 +1,157 @@
+#!/bin/bash
+


It would be good to add an argument to this script to allow it to update the golden results, and document when to use that here. Some changes to vqm2blif will change the output, so being able to quickly update the golden results and check the new ones in will be important.

To match vtr_flow.py (script used for lots of flows), this argument would be named:
-create_golden

Also useful to take a -help argument that prints out the usage of the script.

We use argparse in most (all) of our python scripts, so copying its naming convention is probably best. https://docs.python.org/3/library/argparse.html
--help or -h should print something like:
usage: testvqm2blif.sh ...

All of the requested features are now supported. Please refer to the attached images for sample functionality and a list of available option

s.

…unction and updated commenting to improve clarity of the function.

… function

…ge information. The script now also supports creating a new set of golden netlists (instead of testing).

vaughnbetz · 2021-09-22T14:56:32Z

Looks good -- thanks @Srivat97 !

Srivat97 added 8 commits September 9, 2021 11:52

made changes so that the node removal function can be used to remove …

d808e99

…any type of nodes

added a seperate function to reorganize module node array after delet…

6a4f91f

…ing nodes

made the remove_node function accessible outside of cleanup.cpp and a…

0f63c41

…lso updated prototypes to match existing convention

fixin redundant declaration of the remove_node function

5aabcdd

added a test script that can be used to verify the vqm2blif program

f294690

added a suite of test benchmarks that can be used when verifying the …

84e12d8

…vqm2blif program

Improved readibility of the vqm2blif program test script output. Also…

f2b9e79

… replaced a string literal with its variable representation.

Added a new job to the github workflow to automatically test the vqm2…

9c35fc6

…blif program.

Srivat97 requested a review from vaughnbetz September 15, 2021 23:19

github-actions bot added the infra Project Infrastructure label Sep 15, 2021

Srivat97 and others added 2 commits September 15, 2021 20:18

added additional commenting to better describe the test script usage

e616752

Merge branch 'master' into reorganizing_vqm2blif_source_code

25dfbd7

vaughnbetz requested changes Sep 17, 2021

View reviewed changes

Srivat97 added 4 commits September 20, 2021 22:44

removed unnecessary parameters from the reorganize_module_node_list f…

5f98b6b

…unction and updated commenting to improve clarity of the function.

removed unnecessary function parameters from the remove_one_lut_nodes…

0bec813

… function

added general comments to the functions in cleanup.h

0e7383c

updated vqm2blif test script to now allow input flags and provide usa…

37d2e42

…ge information. The script now also supports creating a new set of golden netlists (instead of testing).

vaughnbetz merged commit 25460d0 into master Sep 22, 2021

vaughnbetz deleted the reorganizing_vqm2blif_source_code branch September 22, 2021 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reorganizing vqm2blif source code #1850

Reorganizing vqm2blif source code #1850

Uh oh!

Srivat97 commented Sep 15, 2021 •

edited

Loading

Uh oh!

vaughnbetz commented Sep 17, 2021

Uh oh!

vaughnbetz Sep 17, 2021

Uh oh!

Srivat97 Sep 21, 2021

Uh oh!

vaughnbetz Sep 17, 2021

Uh oh!

Srivat97 Sep 21, 2021

Uh oh!

Srivat97 Sep 21, 2021

Uh oh!

vaughnbetz Sep 17, 2021

Uh oh!

Srivat97 Sep 21, 2021

Uh oh!

vaughnbetz Sep 17, 2021

Uh oh!

vaughnbetz Sep 17, 2021

Uh oh!

vaughnbetz Sep 17, 2021

Uh oh!

vaughnbetz Sep 17, 2021

Uh oh!

Srivat97 Sep 22, 2021

Uh oh!

vaughnbetz commented Sep 22, 2021

Uh oh!

Uh oh!

Reorganizing vqm2blif source code #1850

Reorganizing vqm2blif source code #1850

Uh oh!

Conversation

Srivat97 commented Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

vaughnbetz commented Sep 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaughnbetz commented Sep 22, 2021

Uh oh!

Uh oh!

Srivat97 commented Sep 15, 2021 •

edited

Loading