8bit shift left register with serial in, serial out implementation in verilog

Question

I took an example from website that describes 8bit serial in serial out shift left register.

module shift (clk, si, so);
input clk,si;
output so;
reg [7:0] tmp;
always @(posedge clk)
begin
 tmp <= tmp << 1;
 tmp[0] <= si;
end
 assign so = tmp[7];
 endmodule

from above example, I think that this is strange implementation. This is because, inside always, when tmp[0] <= si; is executed first, then we will take unexpected output.

For example, let's assume si is 1, and when tmp is:

1010 1010

and, when tmp[0] <= si; is executed first(which will make an unexpected output), the output is:

0101 0110

But, if always statements are executed sequentially as it is written in above example, the output is

0101 0101

As a result, we cannot guarantee that the output could be what we expected.

Am I right? So, the correct implementation should be:

tmp <= (tmp << 1) + {7'b0000_000, si};

Or, can we prove that the above example is always executed sequentially?

• The Photon: The usual way to write a left-shift is tmp <= {tmp[6:0], si};
• sungjun cho: @ThePhoton Do you mean the example above is not correct?
• The Photon: The (second) version you have gives the same answer, but I trust mine to synthesize without generating an unnecessary adder.
• The Photon: The first version you gave should give the same result as mine, but needs an extra line of code and will be harder to read for whoever has to debug your code.

Answer 1

HDL languages do not work like other computer languages. They all have what is called 'concurrent' assignments. Which means all assignments take place 'at the same time'.

Thus tmp[0] gets the value si at the same time as tmp[7:1] get the value from tmp[6:0].

Am I right?

No, I strongly suggest you study Verilog and VHDL in more detail.

---

I think I see where you are coming from. In Verilog when there are multiple concurrent assignments, the last one wins.

Thus tmp[0] gets assigned a zero: tmp <= tmp << 1;
Next it gets assigned si: tmp[0] <= si;
The last one overrides the previous.

I agree it is sloppy coding here.

However there are cases where using that technique greatly improves code readability. I always add a comment like this:

...
... // big complex FSM 
...
// Overrides all previous assignments above! 
if (emergency_stop)
 state <= FSM_IDLE;

• sungjun cho: I know they actually run concurrently. But to get correct value, at the time of tmp[0] <= si, tmp should be shifted first. So, because they run concurrently, we cannot guarantee such action.
• Jules: @sungjuncho - no, the fact that they run concurrently means that effectively the result of every assignment is determined before any actual assignment takes place. tmp[7:1] is determined entirely by the value of tmp[6:0] in the instant before the assignment of tmp[0] takes place; then they all change concurrently. It's a bad way of writing it because it isn't clear, but it is entirely deterministic.
• Jules: Or another way of looking at it: on your clock edge, all of the values referenced in the block are instantaneously sampled; then the assignments take place using the values as they were when they were sampled, and then only once all of the assignments have been processed is anything allowed to view the results. Because the tmp[0] assignment is written after the tmp assignment, bit 0 is excluded from the change to tmp.