always module in Verilog RTL file not working, but working once included in testbench

Question

This might seem like a very naive question, but I have just started working with Verilog (I use Xilinx ISE, if that helps).

I am trying to implement a shift register that shifts input PI by the value specified in the shft port. When I include the shifting logic in the RTL file, the shifting does not work, but when I move the always block corresponding to shifting to the testbench, it works. Please help me with this!

module shift (PI, shft, clk, PO);
input  [7:0] PI;
input clk;
input [7:0] shft; 
output reg [13:0] PO;
reg [7:0] shft_reg;
always @(posedge clk) begin
  if  (shft_reg[0]||shft_reg[1]||shft_reg[2]||shft_reg[3]||shft_reg[4]||shft_reg[5]||shft_reg[6]||shft_reg[7]) begin 
      PO <= {PO, 0};
      shft_reg <= shft_reg-1;
  end 
end
endmodule

I have initialized PO to PI in my testbench. It is kind of iffy, true. — SulfoCyaNate
– SulfoCyaNate, Commented Mar 19, 2015 at 18:51

mcleod_ideafix · Accepted Answer · 2015-03-18 13:18:37Z

1

module shift (
    input wire clk;
    input wire load;  // load shift register from input
    input wire [7:0] PI;
    input wire [7:0] shft; // this might need less bits
    output wire [13:0] PO;
    );

    reg [7:0] shft_reg;
    reg [13:0] value;
    assign PO = value; // PO follows value 
    always @(posedge clk) begin
      if (load) begin  // initialize shift register and counter
        shft_reg <= shft;
        value <= {6'b0,PI};
      end
      else if (shft_reg) begin // if counter not reached end...
        shft_reg <= shft_reg - 1;  // decrement, and
        value <= {value[13:1],1'b0};  // shift left value 1 bit
      end
  end 
end
endmodule

Recall that Verilog supports the >> and << operators. For non-constants many-bit operands, this may be a waste of multiplexers, though:

module shiftcomb (
    input wire [7:0] PI;   // this value is left shifted
    input wire [2:0] shft; // 0 to 7 bits positions
    output wire [14:0] PO; // and is outputted to PO
    );

    assign PO = PI<<shft; // this will generate 15 mutlplexers:
                          // each one with 8 inputs, 3 bit select,
                          // and 1 output.
endmodule

answered Mar 18, 2015 at 13:18

mcleod_ideafix

11.5k2 gold badges30 silver badges33 bronze badges

Sign up to request clarification or add additional context in comments.

6 Comments

Morgan Over a year ago

You have if (shft_reg) where shft_reg is multi bit I would consider this bad practice.

mcleod_ideafix Over a year ago

Why? It's the same than if (shft_reg != 0), isn't it?

Morgan Over a year ago

I would consider comparing to 0 bad practice as well. I would always size the comparison == 8'd0. Then you get the correct warnings regarding width mismatch. It stops confusing a mutli-bit with a single bit enable and I find it aids code readability. In this case I would consider shft_reg != 8'd0 to be the best option.

SulfoCyaNate Over a year ago

Thank you! I would have upvoted this, if I had sufficient reputation. I implemented it this way as well, but I had a general question - can I never have a sequential logic block (always) in a module, and then use it when I instantiate it. In this case, a purely combinational logic block worked, but say, I were forced to use sequential logic (inserting a delay block, for instance), how would I go about it?

mcleod_ideafix Over a year ago

I'm afraid I don't understand your question. Can you elaborate an example? You say you were forced to use sequential logic, so the second solution doesn't suit you, but the first one does use sequential logic.

|

Morgan · Accepted Answer · 2015-03-18 14:25:39Z

0

Note that || is a logical or and idealy should be used with logical statments such as (shft_reg[0] == 1'b1 ) || ( shft_reg[1] == 1'b1).

Your if statment is really bitwise ORing all of the bits ie

shft_reg[0] | shft_reg[1] | shft_reg[2] | ...

You can use the OR Reduction operator :

|shft_reg

Your supplied code had typo'd PI for PO.

always @(posedge clk) begin
  if  (|shft_reg) begin 
      PO       <= {PI, 0}; //PI input 
      shft_reg <= shft_reg-1;
  end 
end

edited Mar 18, 2015 at 14:25

answered Mar 18, 2015 at 8:34

Morgan

20.7k8 gold badges62 silver badges87 bronze badges

1 Comment

mcleod_ideafix Over a year ago

This is bad for many reasons: shft_reg is not meant to be shifted, but substracted. The OP stated that he wants to shift PI by the value specified in shft_reg. So it must be decremented on every clock cycle. Second: PO doesn'0t get the value of PI shifted, but receives always the same value, which is PI with a bit 0 appended to it. Thrid: shft_reg is meant to be compared against 0 to see if there's more to shift, so compare against 0 and let the synthesizer to decide if it is better to infer a N input OR gate, or a comparator: whatever it is more well suited for the target architecture.

Collectives™ on Stack Overflow

always module in Verilog RTL file not working, but working once included in testbench

2 Answers 2

6 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

6 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related