How to simplify sequential logic design by eliminating nested if-else statements

Question

I have a design I've implemented using vhdl that is triggered based on a clock that sends an input signal to one of 8 output channels based on the sel input and also another 2 bit input. The elaborated design shows a lot of nesting due to the many if-else statements. So, I'm curious as to if there's a way to alleviate the nesting using case statements or some other method. I'm unfamiliar with doing this using case statements because I have two inputs that determine the output channel. The code that I currently have is displayed below.

library IEEE;
use IEEE.STD_LOGIC_1164.ALL;


--Inputs and outputs to entity 
entity DSA_Worker is
    Port ( DB_Select : in STD_LOGIC;
           Atten : in STD_LOGIC_VECTOR ( 7 downto 0);
           enable : in STD_LOGIC;
           clk: in STD_LOGIC;
           reset: in STD_LOGIC;
           chip_select : in STD_LOGIC_VECTOR (1 downto 0);
           DBA_TX1_DSA : out STD_LOGIC_VECTOR (7 downto 0);
           DBA_TX2_DSA : out STD_LOGIC_VECTOR (7 downto 0);
           DBA_RX1_DSA : out STD_LOGIC_VECTOR (7 downto 0);
           DBA_RX2_DSA : out STD_LOGIC_VECTOR (7 downto 0);
           DBB_TX1_DSA : out STD_LOGIC_VECTOR (7 downto 0);
           DBB_TX2_DSA : out STD_LOGIC_VECTOR (7 downto 0);
           DBB_RX1_DSA : out STD_LOGIC_VECTOR (7 downto 0);
           DBB_RX2_DSA : out STD_LOGIC_VECTOR (7 downto 0));
end DSA_Worker;

architecture Behavioral of DSA_Worker is

begin
process(clk)
begin
  if rising_edge(clk) then
  --if reset is high, all outputs go to 0
     if reset = '1'then
       DBA_TX1_DSA <= (others =>'0');
       DBA_TX2_DSA <= (others =>'0');
       DBA_RX1_DSA <= (others =>'0');
       DBA_RX2_DSA <= (others =>'0');
       DBB_TX1_DSA <= (others =>'0');
       DBB_TX2_DSA <= (others =>'0');
       DBB_RX1_DSA <= (others =>'0');
       DBB_RX2_DSA <= (others =>'0');
        
          
      -- attenuation values sent to channels based on DB_select value(0/1) and chip select value
       --DB_select - 0 is for DB-A and 1 for DB- B and chip_select determines the channel 
       elsif enable = '1'then
             if(DB_Select='0' and chip_select = "00") then -- attenuation value sent to first channel
             DBA_TX1_DSA(0) <= Atten(0);
             DBA_TX1_DSA(1) <= Atten(1);
             DBA_TX1_DSA(2) <= Atten(2);
             DBA_TX1_DSA(3) <= Atten(3);
             DBA_TX1_DSA(4) <= Atten(4);
             DBA_TX1_DSA(5) <= Atten(5);

             elsif (DB_Select='0' and chip_select = "01") then 
             DBA_TX2_DSA(0) <= Atten(0);
             DBA_TX2_DSA(1) <= Atten(1);
             DBA_TX2_DSA(2) <= Atten(2);
             DBA_TX2_DSA(3) <= Atten(3);
             DBA_TX2_DSA(4) <= Atten(4);
             DBA_TX2_DSA(5) <= Atten(5);
        
             elsif (DB_Select='0' and chip_select = "10") then
             DBA_RX1_DSA(0) <= Atten(0);
             DBA_RX1_DSA(1) <= Atten(1);
             DBA_RX1_DSA(2) <= Atten(2);
             DBA_RX1_DSA(3) <= Atten(3);
             DBA_RX1_DSA(4) <= Atten(4);
             DBA_RX1_DSA(5) <= Atten(5);
  
            elsif (DB_Select='0' and chip_select = "11") then
            DBA_RX2_DSA(0) <= Atten(0);
            DBA_RX2_DSA(1) <= Atten(1);
            DBA_RX2_DSA(2) <= Atten(2);
            DBA_RX2_DSA(3) <= Atten(3);
            DBA_RX2_DSA(4) <= Atten(4);
            DBA_RX2_DSA(5) <= Atten(5);
      
            -- Attenuation values being set for DB-B
            elsif (DB_Select='1' and chip_select = "00") then 
            DBB_TX1_DSA(0) <= Atten(0);
            DBB_TX1_DSA(1) <= Atten(1);
            DBB_TX1_DSA(2) <= Atten(2);
            DBB_TX1_DSA(3) <= Atten(3);
            DBB_TX1_DSA(4) <= Atten(4);
            DBB_TX1_DSA(5) <= Atten(5);
       
            elsif (DB_Select='1' and chip_select = "01") then 
            DBB_TX2_DSA(0) <= Atten(0);
            DBB_TX2_DSA(1) <= Atten(1);
            DBB_TX2_DSA(2) <= Atten(2);
            DBB_TX2_DSA(3) <= Atten(3);
            DBB_TX2_DSA(4) <= Atten(4);
            DBB_TX2_DSA(5) <= Atten(5);
 
            elsif (DB_Select='1' and chip_select = "10") then 
            DBB_RX1_DSA(0) <= Atten(0);
            DBB_RX1_DSA(1) <= Atten(1);
            DBB_RX1_DSA(2) <= Atten(2);
            DBB_RX1_DSA(3) <= Atten(3);
            DBB_RX1_DSA(4) <= Atten(4);
            DBB_RX1_DSA(5) <= Atten(5);
       
         else 
         DBB_RX2_DSA(0) <= Atten(0);
         DBB_RX2_DSA(1) <= Atten(1);
         DBB_RX2_DSA(2) <= Atten(2);
         DBB_RX2_DSA(3) <= Atten(3);
         DBB_RX2_DSA(4) <= Atten(4);
         DBB_RX2_DSA(5) <= Atten(5);
            
            end if; 
        end if;
     end if;

end process;

end Behavioral;

I did try to produce a simplified design using case statements as illustrated below

library IEEE;
use IEEE.STD_LOGIC_1164.ALL;

-- Uncomment the following library declaration if using
-- arithmetic functions with Signed or Unsigned values
--use IEEE.NUMERIC_STD.ALL;

-- Uncomment the following library declaration if instantiating
-- any Xilinx leaf cells in this code.
--library UNISIM;
--use UNISIM.VComponents.all;

entity dsa_case is
   Port ( 
           DB_Select : in STD_LOGIC;
           Atten : in STD_LOGIC_VECTOR ( 7 downto 0);
           enable : in STD_LOGIC;
           clk: in STD_LOGIC;
           reset: in STD_LOGIC;
           chip_select : in STD_LOGIC_VECTOR (1 downto 0);
           DBA_TX1_DSA : out STD_LOGIC_VECTOR (7 downto 0);
           DBA_TX2_DSA : out STD_LOGIC_VECTOR (7 downto 0);
           DBA_RX1_DSA : out STD_LOGIC_VECTOR (7 downto 0);
           DBA_RX2_DSA : out STD_LOGIC_VECTOR (7 downto 0);
           DBB_TX1_DSA : out STD_LOGIC_VECTOR (7 downto 0);
           DBB_TX2_DSA : out STD_LOGIC_VECTOR (7 downto 0);
           DBB_RX1_DSA : out STD_LOGIC_VECTOR (7 downto 0);
           DBB_RX2_DSA : out STD_LOGIC_VECTOR (7 downto 0));
end dsa_case;

architecture Behavioral of dsa_case is

begin

process(clk)
begin

    if rising_edge(clk) then
       if reset = '1' then
        DBA_TX1_DSA <= (others =>'0');
        DBA_TX2_DSA <= (others =>'0');
        DBA_RX1_DSA <= (others =>'0');
        DBA_RX2_DSA <= (others =>'0');
        DBB_TX1_DSA <= (others =>'0');
        DBB_TX2_DSA <= (others =>'0');
        DBB_RX1_DSA <= (others =>'0');
        DBB_RX2_DSA <= (others =>'0');
       
       
     elsif enable = '1'then
        --    DB_Select <='0';
            case chip_select is
             when "00"=>   -- attenuation value sent to first channel
             DBA_TX1_DSA(0) <= Atten(0);
             DBA_TX1_DSA(1) <= Atten(1);
             DBA_TX1_DSA(2) <= Atten(2);
             DBA_TX1_DSA(3) <= Atten(3);
             DBA_TX1_DSA(4) <= Atten(4);
             DBA_TX1_DSA(5) <= Atten(5);

             when "01"=>
             DBA_TX2_DSA(0) <= Atten(0);
             DBA_TX2_DSA(1) <= Atten(1);
             DBA_TX2_DSA(2) <= Atten(2);
             DBA_TX2_DSA(3) <= Atten(3);
             DBA_TX2_DSA(4) <= Atten(4);
             DBA_TX2_DSA(5) <= Atten(5);
        
            when "10"=>
             DBA_RX1_DSA(0) <= Atten(0);
             DBA_RX1_DSA(1) <= Atten(1);
             DBA_RX1_DSA(2) <= Atten(2);
             DBA_RX1_DSA(3) <= Atten(3);
             DBA_RX1_DSA(4) <= Atten(4);
             DBA_RX1_DSA(5) <= Atten(5);
  
            when "11"=>
            DBA_RX2_DSA(0) <= Atten(0);
            DBA_RX2_DSA(1) <= Atten(1);
            DBA_RX2_DSA(2) <= Atten(2);
            DBA_RX2_DSA(3) <= Atten(3);
            DBA_RX2_DSA(4) <= Atten(4);
            DBA_RX2_DSA(5) <= Atten(5);
      
            -- Attenuation values being set for DB-B
      --      DB_Select <= '1';
            when "00"=>
            DBB_TX1_DSA(0) <= Atten(0);
            DBB_TX1_DSA(1) <= Atten(1);
            DBB_TX1_DSA(2) <= Atten(2);
            DBB_TX1_DSA(3) <= Atten(3);
            DBB_TX1_DSA(4) <= Atten(4);
            DBB_TX1_DSA(5) <= Atten(5);
       
      --      DB_Select <= '1';
            when "01" =>
            DBB_TX2_DSA(0) <= Atten(0);
            DBB_TX2_DSA(1) <= Atten(1);
            DBB_TX2_DSA(2) <= Atten(2);
            DBB_TX2_DSA(3) <= Atten(3);
            DBB_TX2_DSA(4) <= Atten(4);
            DBB_TX2_DSA(5) <= Atten(5);
 
     --       DB_Select <= '1';
            when "10"=> 
            DBB_RX1_DSA(0) <= Atten(0);
            DBB_RX1_DSA(1) <= Atten(1);
            DBB_RX1_DSA(2) <= Atten(2);
            DBB_RX1_DSA(3) <= Atten(3);
            DBB_RX1_DSA(4) <= Atten(4);
            DBB_RX1_DSA(5) <= Atten(5);
       
         when others=>
         DBB_RX2_DSA(0) <= Atten(0);
         DBB_RX2_DSA(1) <= Atten(1);
         DBB_RX2_DSA(2) <= Atten(2);
         DBB_RX2_DSA(3) <= Atten(3);
         DBB_RX2_DSA(4) <= Atten(4);
         DBB_RX2_DSA(5) <= Atten(5);
         
        end case;
        
      end if;
    end if; 
 end process;


end Behavioral;

However, it did fail to produce an elaborated design due to 5 errors, all like this

[Synth 8-517] overlapping choice 2'b00 in case statement ["/home/n310-osp/case_statement_design/case_statement_design.srcs/sources_1/new/dsa_case.vhd":108]

The error says it all: you have duplicated cases. Consider to include `DB_Select` in the expression, for example by combining it with `chip_select`. -- Why do you only assign indices 0 to 5, and not 6 and 7? Did you consider to simplify these six assignments into one? (Look into your VHDL book how to do this.) — the busybee, Jun 02 '21 at 06:40
Why do you want to simplify this? Is it just for readability reasons? Because if you are doing synthesis, this will be simple, because it would end up in some multiplexers. — Bananenkönig, Jun 02 '21 at 06:43
Using if or case should make no difference to the logic if all the choices are mutually exclusive, like in case. — Tricky, Jun 02 '21 at 07:05
If the choices are mutually exclusive I'd recommend using case instead of if-elsif. On FPGAs the effect is hardly noticeable because LUT input width but on ASIC gates are discrete so it's a worse option on terms of area and timing. In the end, it's all about building good habits :) — suoto, Jun 02 '21 at 20:57

score 1 · Accepted Answer · answered Jun 02 '21 at 09:05

Personally I don't mind your if-else structure. I think you mostly have a readability issue, with a lot of redundancy (in this case the check on DB_Select inside each case) and inconsistent indentation.

This is how I'd improve the code inside the process:

process(clk)
begin
  if rising_edge(clk) then
    --if reset is high, all outputs go to 0
    if reset = '1' then
      DBA_TX1_DSA <= (others=>'0');
      DBA_TX2_DSA <= (others=>'0');
      DBA_RX1_DSA <= (others=>'0');
      DBA_RX2_DSA <= (others=>'0');
      DBB_TX1_DSA <= (others=>'0');
      DBB_TX2_DSA <= (others=>'0');
      DBB_RX1_DSA <= (others=>'0');
      DBB_RX2_DSA <= (others=>'0');
         
    -- attenuation values sent to channels based on DB_select value(0/1) and chip select value
    -- DB_select - 0 is for DB-A and 1 for DB- B and chip_select determines the channel 
    elsif enable = '1'then
      if DB_Select = '0' then
        if chip_select = "00" then -- attenuation value sent to first channel
          DBA_TX1_DSA(5 downto 0) <= Atten(5 downto 0);
        elsif chip_select = "01" then 
          DBA_TX2_DSA(5 downto 0) <= Atten(5 downto 0);
        elsif chip_select = "10" then
          DBA_RX1_DSA(5 downto 0) <= Atten(5 downto 0);
        else
          DBA_RX2_DSA(5 downto 0) <= Atten(5 downto 0);
        end if;
      else -- Attenuation values being set for DB-B
        if chip_select = "00" then 
          DBB_TX1_DSA(5 downto 0) <= Atten(5 downto 0);
        elsif chip_select = "01" then 
          DBB_TX2_DSA(5 downto 0) <= Atten(5 downto 0);
        elsif chip_select = "10" then 
          DBB_RX1_DSA(5 downto 0) <= Atten(5 downto 0);
        else 
          DBB_RX2_DSA(5 downto 0) <= Atten(5 downto 0);
        end if;
      end if;
    end if;
  end if;
end process;

end Behavioral;

You could replace the if-else structure with when-else, but that's just a matter of preference.

I definitely agree @anderswb you're approach improves readability when it comes to the first approach with the if-elsif statements. Ultimately, I improved on the second approach of using the case statements by including the db_select within the chip_select, making it 3 bits that determines the output similar to what one of the commenters suggested. There was no REAL issue with the first approach, however, from class I was taught a lot of nesting can be problematic. That was my biggest motivation for wanting to improve the design. Thanks for the help — Tellrell White, Jun 03 '21 at 13:30

How to simplify sequential logic design by eliminating nested if-else statements

1 Answers1