Adding two binary-coded-decimal numbers

Question

I want to make some VHDL code open source. But before I do, I want to make sure that it is as readable as possible. Things to improve upon could for example be naming and the use of comments.

The code is adding two binary-coded-decimal numbers. The algorithm behind it will come in the documentation. Note that this isn't the default way to add BCD numbers.

library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;
entity BCD_adder is
 generic(
 --BCD_ADD_DEC_SIZE is the amount of decimal digits. e.g. BCD_ADD_DEC_SIZE = 4 means the highest representable integer is 9999
 BCD_ADD_DEC_SIZE : integer := 3
 );
 port(
 --input and output ports
 BCD_add_a_i, BCD_add_b_i : in STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
 BCD_add_sum_o : out STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
 BCD_add_clk_i, BCD_add_cin_i : in STD_LOGIC;
 BCD_add_cout_o : out STD_LOGIC
 );
end entity;
architecture behavioral of BCD_adder is
 signal BCD_a, BCD_b : unsigned(4*BCD_ADD_DEC_SIZE downto 0);
 signal BCD_sum : STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
 signal BCD_cout : STD_LOGIC;
 --increment6: a function to increment a 4 bit STD_LOGIC_VECTOR by 6.
 function increment6 (number : STD_LOGIC_VECTOR(3 downto 0)) return STD_LOGIC_VECTOR is
 begin
 return ((number(3) or number(2) or number(1)) & ((number(2) or number(1)) nand (number(2) nand number(1))) & not(number(1)) & number(0));
 end function;
 --decrement6: a function to decrement a 4 bit STD_LOGIC_VECTOR by 6.
 function decrement6 (number : STD_LOGIC_VECTOR(3 downto 0)) return STD_LOGIC_VECTOR is
 begin
 return ((number(3) and number(2) and number(1)) & (number(2) xor number(1)) & not(number(1)) & number(0)); 
 end function;
begin
 process(BCD_add_clk_i)
 --BCD_SIZE is the amount of binary digits that are needed for the BCD number. Each decimal digit is 4 bits, so 4*BCD_ADD_DEC_SIZE.
 constant BCD_SIZE : integer := 4*BCD_ADD_DEC_SIZE;
 variable cin : STD_LOGIC;
 variable a, b, sum : unsigned(BCD_SIZE downto 0);
 begin
 if rising_edge(BCD_add_clk_i) then
 --Put the inputs in the variables, add a leading '0' to store a later carry.
 BCD_a <= unsigned('0' & BCD_add_a_i);
 BCD_b <= unsigned('0' & BCD_add_b_i);
 cin := BCD_add_cin_i;
 a := BCD_a;
 b := BCD_b;
 --increment every decimal digit of operand b by 6
 for i in 0 to BCD_ADD_DEC_SIZE-1 loop
 b(4*i+3 downto 4*i) := unsigned(increment6(STD_LOGIC_VECTOR(b(4*i+3 downto 4*i))));
 end loop;
 --add a, b and the carry_in to form a temporary sum that needs to be corrected
 sum := a + b + ("" & cin);
 --correction: if the sum of two decimal digits exceeded 9, subtract 6 from the temporary sum
 for j in 0 to BCD_ADD_DEC_SIZE-1 loop
 if (sum(4*j+4) = (a(4*j+4) xor b(4*j+4))) then
 sum(4*j+3 downto 4*j) := unsigned(decrement6(STD_LOGIC_VECTOR(sum(4*j+3 downto 4*j))));
 end if;
 end loop;
 BCD_sum <= STD_LOGIC_VECTOR(sum(BCD_SIZE-1 downto 0));
 BCD_cout <= sum(BCD_SIZE);
 end if;
 --assign outputs. If the sum has a carry out of '1', BCD_add_cout_o will be set to '1'.
 BCD_add_sum_o <= BCD_sum;
 BCD_add_cout_o <= BCD_cout;
 end process;
end behavioral; 

Answer 1

Overall, its not too bad, but I do have some suggestions for readability. What I like to do is add white-space to make elements line up with each other. For example, for this line:

BCD_add_a_i, BCD_add_b_i : in STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
BCD_add_sum_o : out STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
BCD_add_clk_i, BCD_add_cin_i : in STD_LOGIC;
BCD_add_cout_o : out STD_LOGIC

I'd do something like this:

BCD_add_a_i : in STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0); 
BCD_add_b_i : in STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
BCD_add_sum_o : out STD_LOGIC_VECTOR(4*BCD_ADD_DEC_SIZE-1 downto 0);
BCD_add_clk_i : in STD_LOGIC;
BCD_add_cin_i : in STD_LOGIC;
BCD_add_cout_o : out STD_LOGIC

I think it makes large blocks of text like that a smidge easier to read.

For the increment/decrement functions, I'd split up the big or/and/nand statements with variables. It ultimately doesn't affect much synthesis-wise, but again makes a bit easier to read. I'd also be consistent on whether you use the '&' operator or 'and' operator, rather than switching throughout the statement.

For this statement:

b(4*i+3 downto 4*i) := unsigned(increment6(STD_LOGIC_VECTOR(b(4*i+3 downto 4*i)))); 

I'd find a way to make that a little less ugly, but I'm not strictly sure how. It might have to be one of those necessary evil type of things, but cleaning that up would be really nice. Maybe do the SLV/unsigned typecasting within the increment/decrement functions.

I'm not too fond of the sorta-Hungarian notation thing for ports and signals, but that's more a personal thing than a readability thing.

• mkrieger1: Note that & is not logical "and", but concatenation.
• Drew: mrkrieger: Yep, you're right, I got C syntax and VHDL syntax confused.

Answer 2

Bug

The cin input is not combined with the a and b inputs of the same cycle, but of the previous cycle.

For example, for the input sequence

a b cin
---------------
0010 0000 0
0020 0000 1
0030 0000 0

the output sequence is 0011, 0020, 0030 instead of 0010, 0021, 0030, as I would expect:

Bug

Don't confuse signals and variables

Confusion about the behaviour of these two kinds of objects is likely the source of the bug.

--Put the inputs in the variables, [...] -- not quite true
BCD_a <= unsigned('0' & BCD_add_a_i);
BCD_b <= unsigned('0' & BCD_add_b_i);
cin := BCD_add_cin_i;
a := BCD_a;
b := BCD_b;

But BCD_a and BCD_b are signals, not variables, so their values are only updated in the next clock cycle. No intermediate signal is used for cin, so it becomes misaligned with the a and b variables.

Unconventional timing

Since there is no internal state on which the outputs depend, the adder could be an entirely combinational circuit and no clock is actually required.

If the circuit should be synchronous to a clock, then it is conventional that the outputs are valid one clock cycle after the inputs, usually by placing registers before the outputs.

In the posted code there are additional registers after some of the inputs, and there is this construct:

process(bcd_add_clk_i)
...
begin
 if rising_edge(bcd_add_clk_i) then
 ...
 bcd_sum <= ...;
 bcd_cout <= ...;
 end if;
 --assign outputs. ...
 bcd_add_sum_o <= bcd_sum;
 bcd_add_cout_o <= bcd_cout;
end process;

This means that bcd_add_sum_o and bcd_add_cout_o get updated when the clock changes (because the assignment is inside the process sensitive to the clock), but not at a rising edge, in other words, at the next falling edge of the clock.

The outputs therefore are delayed by 21⁄2 clock cycles with respect to the inputs, instead of 0 (in the case of combinational logic) or 1 clock cycles, as can be seen in the screenshot above.

What you maybe meant to write was

process(bcd_add_clk_i)
...
begin
 if rising_edge(bcd_add_clk_i) then
 ...
 bcd_sum <= ...;
 bcd_cout <= ...;
 end if;
end process;
--assign outputs. [...] -- outside the clock process
bcd_add_sum_o <= bcd_sum;
bcd_add_cout_o <= bcd_cout;

or simply

process(bcd_add_clk_i)
...
begin
 if rising_edge(bcd_add_clk_i) then
 ...
 bcd_add_sum_o <= ...; -- no intermediate signal
 bcd_add_cout_o <= ...;
 end if;
end process;

Comments, naming, and interface design

Some comments are simply not needed. For example:

port(
 --input and output ports
 ...
);

The port(); syntax is already a pretty strong suggestion that it contains a list of input and output ports.

Other comments could be made unnecessary by using better names. For example:

--bcd_add_dec_size is the amount of decimal digits. [...]
bcd_add_dec_size : integer := 3

Using the name num_digits would already explain its purpose. (Besides, it makes no sense to have fewer than one digit, so it should be a positive, not an integer.)

Using the bcd_ or bcd_add_ prefixes would be appropriate in a higher level of the design hierarchy in order to distinguish names from similar names that do not belong to the BCD adder. However, inside the BCD adder, there is nothing to distinguish and the prefixes just add noise.

The _i and _o suffixes used for port names are fine, because they distinguish the ports from internal signals. I would not use the _i suffix for the clock, however, because it is clear that it is an input.

I would list the clock individually as the first port (so that it becomes obvious that it is a sequential circuit), instead of grouping it with cin. The two ports may have the same type and direction, but conceptually they are different things.

Use better abstractions

Manipulating bits by hand in order to implement arithmetic functions is error-prone and hard to understand and check for correctness by a human. The numeric_std package defines the operators to let you simply write + 6 and - 6 instead of using the increment6 and decrement6 functions. (Also, converting from unsigned to std_logic_vector and back was not necessary.)

Define some types representing the different kinds of values that occur in the design, and corresponding functions to operate on them. This lets you emphasize what the design accomplishes and reduces the possibilities for errors in details about how it is done, for example when counting the indices of a large bit vector in several places.

Suggested solution

This may seem a bit over-engineered for such a simple design, but chances are that you need to deal with BCD numbers in other places, too. Then you can re-use the types and functions by placing them in a package.

Omitting the type conversions at entity boundaries and only doing them at the very top level will then also simplify individual entities.

This should by synthesizable with any VHDL-93 compiler. I tested it with Cadence RTL Compiler version 14, and apart from 24 fewer flip-flops compared to the original code, there is practically no difference in gate count.

library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;
entity bcd_adder is
 generic (
 num_digits : integer := 3);
 port (
 clk : in std_logic;
 a_i, b_i : in std_logic_vector(4 * num_digits - 1 downto 0);
 cin_i : in std_logic;
 sum_o : out std_logic_vector(4 * num_digits - 1 downto 0);
 cout_o : out std_logic);
end entity;
architecture behavioral of bcd_adder is
 subtype bcd_digit is natural range 0 to 9;
 type bcd_number is array (num_digits - 1 downto 0) of bcd_digit;
 constant bits_per_digit : natural := 4;
 subtype bcd_vector is unsigned(bits_per_digit * num_digits - 1 downto 0);
 function from_vector (v : bcd_vector) return bcd_number is
 variable offset : natural;
 variable result : bcd_number;
 begin
 for digit in bcd_number'range loop
 offset := digit * bits_per_digit;
 result(digit) := to_integer(
 v(offset + bits_per_digit - 1 downto offset));
 end loop;
 return result;
 end function;
 function to_vector (num : bcd_number) return bcd_vector is
 variable offset : natural;
 variable result : bcd_vector;
 begin
 for digit in bcd_number'range loop
 offset := digit * bits_per_digit;
 result(offset + bits_per_digit - 1 downto offset) :=
 to_unsigned(num(digit), bits_per_digit);
 end loop;
 return result;
 end function;
 subtype carry is natural range 0 to 1;
 type bcd_sum is record
 s : bcd_number;
 c : carry;
 end record;
 function add_numbers (a, b : bcd_number; c : carry) return bcd_sum is
 variable result : bcd_sum;
 variable digit_sum : natural;
 variable ripple : carry := c;
 begin
 for digit in bcd_number'right to bcd_number'left loop
 digit_sum := a(digit) + b(digit) + ripple;
 if digit_sum >= 10 then
 result.s(digit) := digit_sum - 10;
 ripple := 1;
 else
 result.s(digit) := digit_sum;
 ripple := 0;
 end if;
 end loop;
 result.c := ripple;
 return result;
 end function;
 signal a, b : bcd_number;
 signal cin : carry;
 signal sum : bcd_sum;
begin
 a <= from_vector(unsigned(a_i));
 b <= from_vector(unsigned(b_i));
 cin <= 1 when cin_i = '1' else 0;
 process (clk) is
 begin
 if rising_edge(clk) then
 sum <= add_numbers(a, b, cin);
 end if;
 end process;
 sum_o <= std_logic_vector(to_vector(sum.s));
 cout_o <= '1' when sum.c = 1 else '0';
end architecture;

• gilianzz: Thanks! I had since already fixed one bug and some naming, but your advice is still very useful.
• gilianzz: Also, the increment6 and decrement6 functions improve the speed of the design. From 288.52 MHz to 325.63 MHz
• gilianzz: I copy pasted your suggest solution to see how fast it was, but unfortunately it doesn't show Fmax. That's why I have registers on both the in and outputs.