pipeline code

C

Chris Hinsley

Guest
Folks, is this a reasonable and correct way to express a pipeline ? I'm
deliberating on things like the use of = or <= in the loop. Advise
would be welcome.

localparam DEPTH = 4;
reg [(REG_WIDTH - 1):0] o_wb;
reg [(REG_SEL_BITS - 1):0] o_wbr;
reg [(DEPTH - 1):0] [(REG_WIDTH - 1):0] pipe_wb;
reg [(DEPTH - 1):0] [(REG_SEL_BITS - 1):0] pipe_wbr;
always @(posedge i_clk or negedge i_rst)
begin
if (!i_rst)
begin
pipe_wb <= 0;
pipe_wbr <= 0;
o_wb <= 0;
o_wbr <= 0;
end
else
begin
integer i;
for (i = 0; i < DEPTH; i++)
begin
if (i == (DEPTH - 1))
begin
pipe_wb = wb;
pipe_wbr = wbr;
end
else
begin
pipe_wb = pipe_wb[i + 1];
pipe_wbr = pipe_wbr[i + 1];
end
end
o_wb <= pipe_wb[0];
o_wbr <= pipe_wbr[0];
end
end
 
On Thu, 21 Apr 2011 15:07:53 +0100, Chris Hinsley
<chris.hinsley@gmail.com> wrote:

Folks, is this a reasonable and correct way to express a pipeline ? I'm
deliberating on things like the use of = or <= in the loop. Advise
would be welcome.
Oh Chris, don't do this to me! I'm an old man and my
blood pressure isn't as well controlled as it once was.

localparam DEPTH = 4;
reg [(REG_WIDTH - 1):0] o_wb;
reg [(REG_SEL_BITS - 1):0] o_wbr;
reg [(DEPTH - 1):0] [(REG_WIDTH - 1):0] pipe_wb;
reg [(DEPTH - 1):0] [(REG_SEL_BITS - 1):0] pipe_wbr;
always @(posedge i_clk or negedge i_rst)
begin
if (!i_rst)
begin
pipe_wb <= 0;
pipe_wbr <= 0;
Synthesis error: you've written to pipe_* here with
nonblocking <= assignment, but later you write to it
with blocking = assignment. Given that one of those
assignments is in the reset branch and the other in
the clocked branch, it doesn't really matter. But
most synthesis tools will choke on it.

o_wb <= 0;
o_wbr <= 0;
end
else
begin
integer i;
Syntax error: can't declare variables in an anonymous
begin...end unless you're using SystemVerilog.

for (i = 0; i < DEPTH; i++)
begin
if (i == (DEPTH - 1))
begin
pipe_wb = wb;
pipe_wbr = wbr;
end
else
begin
pipe_wb = pipe_wb[i + 1];
pipe_wbr = pipe_wbr[i + 1];
end
end

OK, you're reading the value of each variable that was
left-over from the previous clock edge, so you're implying
a pipeline (shift register) exactly as you suggest.

And then...

;
o_wbr <= pipe_wbr[0];
end
end
you copy the final value out on to an output register, which
will in fact be a duplication of pipe_*[0]. All good.

But, what's the bit that gave me conniptions? Answer: your
declaration of the pipe_* variables OUTSIDE the clocked
always block. Remember I got a bit hot under the collar,
in a post a few weeks ago, about "never use blocking
assignment in a clocked process to write to a variable
that can be read outside the process" (or something
much like that)? Well, here you've left yourself
wide open to that by declaring those pipe variables
at the outer scope, where any other process can get
its hands on them. MOVE THEIR DECLARATION INTO THE
CLOCKED ALWAYS PROCESS so that they're invisible
outside it. (Note for LRM lawyers later in this post,
but it doesn't affect the real argument.)

That means you'll need to name the outermost begin...end
and put the pipe variable declarations inside it.
Not, of course, the o_wb* variables, which need to
be visible elsewhere and which you're quite properly
writing by nonblocking assignment.

VHDL doesn't let you do this, because its variables
(equivalent to Verilog variables written by = ) can
only be declared within a process. Verilog treats
that as an unacceptable infringement of the programmer's
divine right to screw up at other people's expense, and
allows you to do the Bad Things.

Apart from the syntactic nit-picks, your code is OK
as it is; but I don't regard it as acceptable to
expose yourself to the risk of race conditions
by making the variables visible outside the process.

OK, so here's the LRM-lawyer note: in Verilog all
module-level variables, and all variables declared
inside named begin...end blocks, are static and
globally visible by means of dotted names:

always @(whatever) begin : contains_var
reg local_var;
blah, blah,...
end

always @(whichwhat) begin
$display("I can snoop %b", contains_var.local_var);
end

But synthesis tools will reject such dotted references,
so your variable is at least to some extent hidden and
safe. And even without the protection of a synthesis
tool being picky, it's obviously much harder to make
such a reference thoughtlessly than it would be if the
variable were at module level.

Hope this helps.
--
Jonathan Bromley
 
On 2011-04-21 19:42:16 +0100, Jonathan Bromley said:

On Thu, 21 Apr 2011 15:07:53 +0100, Chris Hinsley
chris.hinsley@gmail.com> wrote:

Folks, is this a reasonable and correct way to express a pipeline ? I'm
deliberating on things like the use of = or <= in the loop. Advise
would be welcome.

Oh Chris, don't do this to me! I'm an old man and my
blood pressure isn't as well controlled as it once was.
I've no sympathy, I'm type 1 diabetic. ;)

localparam DEPTH = 4;
reg [(REG_WIDTH - 1):0] o_wb;
reg [(REG_SEL_BITS - 1):0] o_wbr;
reg [(DEPTH - 1):0] [(REG_WIDTH - 1):0] pipe_wb;
reg [(DEPTH - 1):0] [(REG_SEL_BITS - 1):0] pipe_wbr;
always @(posedge i_clk or negedge i_rst)
begin
if (!i_rst)
begin
pipe_wb <= 0;
pipe_wbr <= 0;

Synthesis error: you've written to pipe_* here with
nonblocking <= assignment, but later you write to it
with blocking = assignment. Given that one of those
assignments is in the reset branch and the other in
the clocked branch, it doesn't really matter. But
most synthesis tools will choke on it.
I thought it would be ok, due as you say to being on the reset path.
But Quartus and ModelSim don't choke on it.

o_wb <= 0;
o_wbr <= 0;
end
else
begin
integer i;

Syntax error: can't declare variables in an anonymous
begin...end unless you're using SystemVerilog.
I had chosen System Verilog on the tool options ! :)

Which I presume isn't a bad thing ?

for (i = 0; i < DEPTH; i++)
begin
if (i == (DEPTH - 1))
begin
pipe_wb = wb;
pipe_wbr = wbr;
end
else
begin
pipe_wb = pipe_wb[i + 1];
pipe_wbr = pipe_wbr[i + 1];
end
end

OK, you're reading the value of each variable that was
left-over from the previous clock edge, so you're implying
a pipeline (shift register) exactly as you suggest.

And then...

o_wb <= pipe_wb[0];
o_wbr <= pipe_wbr[0];
end
end

you copy the final value out on to an output register, which
will in fact be a duplication of pipe_*[0]. All good.

But, what's the bit that gave me conniptions? Answer: your
declaration of the pipe_* variables OUTSIDE the clocked
always block.

Sorry, didn't realise that was a bad thing. But I supose if there not
used outside the always block then they are local.

I bet your going to say 'Don't use the same registers in 2 sepearte
always blocks...' ?


Remember I got a bit hot under the collar,
in a post a few weeks ago, about "never use blocking
assignment in a clocked process to write to a variable
that can be read outside the process" (or something
much like that)? Well, here you've left yourself
wide open to that by declaring those pipe variables
at the outer scope, where any other process can get
its hands on them. MOVE THEIR DECLARATION INTO THE
CLOCKED ALWAYS PROCESS so that they're invisible
outside it. (Note for LRM lawyers later in this post,
but it doesn't affect the real argument.)
Ah, I'd read that before as not to right with = to output registers,
which is why I had the <= to the o_xx reg's.

That means you'll need to name the outermost begin...end
and put the pipe variable declarations inside it.
Not, of course, the o_wb* variables, which need to
be visible elsewhere and which you're quite properly
writing by nonblocking assignment.
Yes, I thought that was the only rule to follow. But I'll move the pipe
vars into the always.

Apart from the syntactic nit-picks, your code is OK
as it is; but I don't regard it as acceptable to
expose yourself to the risk of race conditions
by making the variables visible outside the process.
Niether do I, that's why I asked the question. I want to make it right,
even if the tools give me running hardware as it is. !

Hope this helps.
Sure.

I was going to check what the rule is when you do have 2 always blocks
that accsess the same variable ? And what the effect of different types
of assignment = or <= in those blocks would do.

Best Regards

Chris
 
Synthesis error: you've written to pipe_* here with
nonblocking <= assignment, but later you write to it
with blocking = assignment. Given that one of those
assignments is in the reset branch and the other in
the clocked branch, it doesn't really matter. But
most synthesis tools will choke on it.

I thought it would be ok, due as you say to being on the reset path.
But Quartus and ModelSim don't choke on it.
Intrestingly, when I do move them to being local to the always block,
Quartus does now complain, but ModelSim is still happy. !

Chris
 
On 2011-04-21 21:15:07 +0100, Chris Hinsley said:

Synthesis error: you've written to pipe_* here with
nonblocking <= assignment, but later you write to it
with blocking = assignment. Given that one of those
assignments is in the reset branch and the other in
the clocked branch, it doesn't really matter. But
most synthesis tools will choke on it.

I thought it would be ok, due as you say to being on the reset path.
But Quartus and ModelSim don't choke on it.

Intrestingly, when I do move them to being local to the always block,
Quartus does now complain, but ModelSim is still happy. !

Chris
OK, I changed the code to this:

always @(posedge i_clk or negedge i_rst)
begin
reg [(DEPTH - 1):0] [(REG_WIDTH - 1):0] pipe_wb;
reg [(DEPTH - 1):0] [(REG_SEL_BITS - 1):0] pipe_wbr;

if (!i_rst)
begin
pipe_wb = 0;
pipe_wbr = 0;
end
else
begin
integer i;
for (i = 0; i < DEPTH; i++)
begin
if (i == (DEPTH - 1))
begin
pipe_wb = wb;
pipe_wbr = wbr;
end
else
begin
pipe_wb = pipe_wb[i + 1];
pipe_wbr = pipe_wbr[i + 1];
end
end
end
o_wb = pipe_wb[0];
o_wbr = pipe_wbr[0];
end

And if I do the last 2 output assignments as <= I get this nasty
warning from Quartus.

Critical Warning (10237): Verilog HDL warning at(171): can't infer
register for assignment in edge-triggered always construct because the
clock isn't obvious. Generated combinational logic instead

As it is with =, no warnings. I don't like to have warnings ! I like to
know why it's warning me, then add some code to tell it exactly what I
want, and for it to shut up !

But doing the = seams to break the rule about righting to output regs with <= ?

Not doing your blood pressure any good this is it. :)

Chris
 
On Thu, 21 Apr 2011 21:00:50 +0100, Chris Hinsley wrote:

Synthesis error: you've written to pipe_* here with
nonblocking <= assignment, but later you write to it
with blocking = assignment. Given that one of those
assignments is in the reset branch and the other in
the clocked branch, it doesn't really matter. But
most synthesis tools will choke on it.

I thought it would be ok, due as you say to being on the reset path.
But Quartus and ModelSim don't choke on it.
ModelSim, being a simulator, is duty-bound to process
the entire language. I'm quite surprised that Quartus
bought that - first synth tool I've seen that would.

Syntax error: can't declare variables in an anonymous
begin...end unless you're using SystemVerilog.

I had chosen System Verilog on the tool options ! :)
Which I presume isn't a bad thing ?
No, it's fine, provided your entire tool chain
offers support and you don't mind the old hands
offering the kind of irritatingly unhelpful
comments you got from me :)

But, what's the bit that gave me conniptions? Answer: your
declaration of the pipe_* variables OUTSIDE the clocked
always block.

Sorry, didn't realise that was a bad thing. But I supose if there not
used outside the always block then they are local.
Right, and even just as a matter of good coding style
it surely makes sense to keep local things local.

I bet your going to say 'Don't use the same registers in 2 sepearte
always blocks...' ?
No, far from it. Only this...

Remember I got a bit hot under the collar,
in a post a few weeks ago, about "never use blocking
assignment in a clocked process to write to a variable
that can be read outside the process" (or something
much like that)?
That's the restriction I really care about.

Ah, I'd read that before as not to right with = to output registers,
which is why I had the <= to the o_xx reg's.
"Output" in this context only means "written by this always
block and (potentially) read elsewhere". I don't care
whether the readers are on the other side of a port
connection, or local to the current module - the advice
remains the same.

I don't regard it as acceptable to
expose yourself to the risk of race conditions
by making the variables visible outside the process.

Niether do I, that's why I asked the question. I want to make it right,
even if the tools give me running hardware as it is. !
Right; the problem is that race conditions, aka
nondeterminacy of thread interleaving, are endemic
in Verilog but can easily remain hidden for a long
time if you are (un)lucky. The usual trigger for
discovery of such bugs is shipment to a customer
about ten minutes ago.

I was going to check what the rule is when you do have 2 always blocks
that accsess the same variable ? And what the effect of different types
of assignment = or <= in those blocks would do.
Rule 1: there is no rule. In the Verilog language you can
write to, and read, any variable from anywhere; and you
are solely responsible for dealing with any nondeterminacy
that may occur. Don't necessarily expect your simulations
to match reality or sanity, though. And don't necessarily
expect the code to be synthesisable.

Rule 2: If you're trying to use simulation to see how
your synthesisable code works, and you want synthesis
tools to make sense of it, you better follow some
guidelines that avoid any need to worry about non-
determinacy.

And the Rule 2 rules for synthesisable logic are, roughly:

1. Only one driver on a variable
================================
For synthesis, a variable better have only one process
driving it. "Process" basically means "always block",
in Verilog. The problem here is that Verilog variables
have last-write-wins semantics, and there's no way to
mimic that in hardware across multiple concurrent
processes. So make sure that any variable is written
by exactly one always block. Within that block you
can, of course, write to the variable as many times
as you please. The variable's value after the clock
is determined by the last of those write operations.
Since all those operations happen at the same moment
of simulation time, there's no problem for a tool
to implement that in hardware.

2. Clocked processes write to non-local vars using <=
=====================================================
When a clocked always block writes to a variable and
that variable is read by any other process, the
write must be <=. This ensures that any reading
process triggered by the same clock edge is sure
to see the pre-clock value of the variable, just as
would happen with a real flipflop. If you use = to
do the write, the other block might see the updated
value instead; this is the Verilog-race equivalent
of a hold time violation, and is a sure-fire way to
make your simulations untrustworthy.

3. Combinational processes write to any vars using =
====================================================
This rule isn't 100% essential, but makes it much
easier to describe clock gating and certain other
kinds of logic, and it never does any harm. Other
combinational processes are triggered by value
changes on their inputs, not by a clock edge, and
therefore will re-evaluate as necessary until they
deal with the final settled values of their inputs;
so the race condition described in (2) cannot occur.

4. Combinational processes must never imply memory
==================================================
Any combinational (always @*, always_comb) process
must represent calculation of outputs as a pure
function of inputs. There must be no storage.
Many of us find the easiest way to ensure that is
to make a default assignment to every output variable
at the start of the process, and then overwrite the
variables as needed during the life of the process
as a function of the input values. You are free
to use local (preferably locally-declared) variables
in such a process to help with any intermediate
computation, but you must never make use of the
leftover value of such variables at the start of
the process's execution. Take care to write to
them before attempting to read their value.

5. Clocked processes write to local vars using either
=====================================================
A local variable in a clocked process (i.e. one that
is not read by any other process, preferably enforced
by declaring it within the process's local scope) can
be written using either = or <= according to need.
Bear in mind that = assignment updates immediately,
so that subsequent reads of the variable see the new
value; this usually implies some additional combinational
logic before whatever flipflop finally captures the
variable's value. <= assignment updates after all
other activity at the moment of the clock edge has
quietened down, so if you read that variable later
in the process's execution you'll see the value as it
was before the clock edge, not the newly updated value;
this means that the variable will certainly be held
in a flipflop, and the flop's value after the clock
edge will be determined by the last thing you wrote
to it using <=. Either way, you must be consistent;
all assignments to a variable should use the same
operator.

That's it, pretty much. All else is a matter of
you, and the synthesis tool, working out what the
code will do. Justifying those guidelines would
take a little longer, but I hope they make some
sense as they stand.

SystemVerilog has some new process constructs that
help you with some, but not all, these guidelines.
For example, always_comb and always_ff enforce the
single-driver rule, and act as hints to simulators
to allow them to warn you about other breaches of
synthesis guidelines.
--
Jonathan Bromley
 
On Thu, 21 Apr 2011 21:29:12 +0100, Chris Hinsley wrote:

OK, I changed the code to this:

always @(posedge i_clk or negedge i_rst)
begin
reg [(DEPTH - 1):0] [(REG_WIDTH - 1):0] pipe_wb;
reg [(DEPTH - 1):0] [(REG_SEL_BITS - 1):0] pipe_wbr;

if (!i_rst)
begin
pipe_wb = 0;
pipe_wbr = 0;
end
else
begin
[do stuff with pipe_*]
end
o_wb = pipe_wb[0];
o_wbr = pipe_wbr[0];
end

And if I do the last 2 output assignments as <= I get this nasty
warning from Quartus.

Critical Warning (10237): Verilog HDL warning at(171): can't infer
register for assignment in edge-triggered always construct because the
clock isn't obvious. Generated combinational logic instead
yes, but now you've moved the assignments to o_* outside
the clocked branch of the always block!

See my other, longer post. Come back and complain if
it's still unclear. You were basically right first time -
it was only the non-locality of the pipe vars that really
bothered me.

Not doing your blood pressure any good this is it. :)
A battle long ago lost :)
--
Jonathan Bromley
 
On 2011-04-21 22:05:19 +0100, Jonathan Bromley said:

On Thu, 21 Apr 2011 21:29:12 +0100, Chris Hinsley wrote:

OK, I changed the code to this:

always @(posedge i_clk or negedge i_rst)
begin
reg [(DEPTH - 1):0] [(REG_WIDTH - 1):0] pipe_wb;
reg [(DEPTH - 1):0] [(REG_SEL_BITS - 1):0] pipe_wbr;

if (!i_rst)
begin
pipe_wb = 0;
pipe_wbr = 0;
end
else
begin
[do stuff with pipe_*]
end
o_wb = pipe_wb[0];
o_wbr = pipe_wbr[0];
end

And if I do the last 2 output assignments as <= I get this nasty
warning from Quartus.

Critical Warning (10237): Verilog HDL warning at(171): can't infer
register for assignment in edge-triggered always construct because the
clock isn't obvious. Generated combinational logic instead

yes, but now you've moved the assignments to o_* outside
the clocked branch of the always block!

See my other, longer post. Come back and complain if
it's still unclear. You were basically right first time -
it was only the non-locality of the pipe vars that really
bothered me.
So, just to be clear, do you now think I should move the pipe vars
outside the always block and do the right to o_xxx with an <= ? Doing
the <= should take priority over the localizing of the pipe vars ?

And thanks for the other msg BTW.

Chris
 
On Thu, 21 Apr 2011 22:22:23 +0100, Chris Hinsley wrote:


So, just to be clear, do you now think I should move the pipe vars
outside the always block
No.

and do the right to o_xxx with an <= ?
Yes.

Doing the <= should take priority over the
localizing of the pipe vars ?
Yes, but it should be OK to do both.

Night-night!
--
Jonathan Bromley
 
On 2011-04-21 22:22:23 +0100, Chris Hinsley said:

On 2011-04-21 22:05:19 +0100, Jonathan Bromley said:

On Thu, 21 Apr 2011 21:29:12 +0100, Chris Hinsley wrote:

OK, I changed the code to this:

always @(posedge i_clk or negedge i_rst)
begin
reg [(DEPTH - 1):0] [(REG_WIDTH - 1):0] pipe_wb;
reg [(DEPTH - 1):0] [(REG_SEL_BITS - 1):0] pipe_wbr;

if (!i_rst)
begin
pipe_wb = 0;
pipe_wbr = 0;
end
else
begin
[do stuff with pipe_*]
end
o_wb = pipe_wb[0];
o_wbr = pipe_wbr[0];
end

And if I do the last 2 output assignments as <= I get this nasty
warning from Quartus.

Critical Warning (10237): Verilog HDL warning at(171): can't infer
register for assignment in edge-triggered always construct because the
clock isn't obvious. Generated combinational logic instead

yes, but now you've moved the assignments to o_* outside
the clocked branch of the always block!

See my other, longer post. Come back and complain if
it's still unclear. You were basically right first time -
it was only the non-locality of the pipe vars that really
bothered me.

So, just to be clear, do you now think I should move the pipe vars
outside the always block and do the right to o_xxx with an <= ? Doing
the <= should take priority over the localizing of the pipe vars ?

And thanks for the other msg BTW.

Chris
This code dosn't give warnings and does <=, and has local pipe vars.
But (with my software hat on), I don't see why I can't move the common
code for the output <= to a single point after the branch without
Quartus giveing me the critical warning. To me, it should still be
obvious to the compiler that there is a continuos assignment to the
output from the end of the pipe.

always @(posedge i_clk or negedge i_rst)
begin
reg [(DEPTH - 1):0] [(REG_WIDTH - 1):0] pipe_wb;
reg [(DEPTH - 1):0] [(REG_SEL_BITS - 1):0] pipe_wbr;

if (!i_rst)
begin
pipe_wb = 0;
pipe_wbr = 0;
o_wb <= pipe_wb[0];
o_wbr <= pipe_wbr[0];
end
else
begin
integer i;
for (i = 0; i < DEPTH; i++)
begin
if (i == (DEPTH - 1))
begin
pipe_wb = wb;
pipe_wbr = wbr;
end
else
begin
pipe_wb = pipe_wb[i + 1];
pipe_wbr = pipe_wbr[i + 1];
end
end
o_wb <= pipe_wb[0];
o_wbr <= pipe_wbr[0];
end
end

Regards

Chris
 
On Thu, 21 Apr 2011 22:34:33 +0100, Chris Hinsley wrote:

This code
[copied at the end of this post]
dosn't give warnings and does <=, and has local pipe vars.
and is what I would have written myself.

But (with my software hat on), I don't see why I can't move the common
code for the output <= to a single point after the branch without
Quartus giveing me the critical warning. To me, it should still be
obvious to the compiler that there is a continuos assignment to the
output from the end of the pipe.
Agreed. As a matter of interest, could you try it with all
the assignments to pipe_* changed to be <= ? I don't think
that would alter the functionality in this case, because you
don't read those variables after writing them - you only
make use of the old, pre-clock value in each case.

I've seen people do the thing you're suggesting - moving
the common assignment outside the if statement - but I
don't do that myself, having had tool problems with it
in the (distant) past. So I can't comment on how
widely supported it is, nor on what the restrictions
might be. My ultra-conservative stance is that it
doesn't fit the standard clocked process template, so
I avoid it. Anything for a quiet life.

always @(posedge i_clk or negedge i_rst)
begin
reg [(DEPTH - 1):0] [(REG_WIDTH - 1):0] pipe_wb;
reg [(DEPTH - 1):0] [(REG_SEL_BITS - 1):0] pipe_wbr;

if (!i_rst)
begin
pipe_wb = 0;
pipe_wbr = 0;
o_wb <= pipe_wb[0];
o_wbr <= pipe_wbr[0];
end
else
begin
integer i;
for (i = 0; i < DEPTH; i++)
begin
if (i == (DEPTH - 1))
begin
pipe_wb = wb;
pipe_wbr = wbr;
end
else
begin
pipe_wb = pipe_wb[i + 1];
pipe_wbr = pipe_wbr[i + 1];
end
end
o_wb <= pipe_wb[0];
o_wbr <= pipe_wbr[0];
end
end

--
Jonathan Bromley
 
Agreed. As a matter of interest, could you try it with all
the assignments to pipe_* changed to be <= ? I don't think
that would alter the functionality in this case, because you
don't read those variables after writing them - you only
make use of the old, pre-clock value in each case.
Yes, I wanted to code it with just <= all along but hit trouble and got
confused.

But, interesting result if I do put it all to <=, Quartus dosn't
complain, but it (at the RTL level) has the output as an extra stage in
the pipeline !!! Which is definately not what I want. This is the code
that ends up with an extra pipe stage.

always @(posedge i_clk or negedge i_rst)
begin
reg [(DEPTH - 1):0] [(REG_WIDTH - 1):0] pipe_wb;
reg [(DEPTH - 1):0] [(REG_SEL_BITS - 1):0] pipe_wbr;

if (!i_rst)
begin
pipe_wb <= 0;
pipe_wbr <= 0;
o_wb <= 0;
o_wbr <= 0;
end
else
begin
integer i;
for (i = 0; i < DEPTH; i++)
begin
if (i == (DEPTH - 1))
begin
pipe_wb <= wb;
pipe_wbr <= wbr;
end
else
begin
pipe_wb <= pipe_wb[i + 1];
pipe_wbr <= pipe_wbr[i + 1];
end
end
o_wb <= pipe_wb[0];
o_wbr <= pipe_wbr[0];
end
end

I can just about see why it might do that compared to the previous
code, but it's not obvious.

All good learning fun this ! :)

Chris
 
I can just about see why it might do that compared to the previous
code, but it's not obvious.

All good learning fun this ! :)

Chris
My thoughts are that it can't do a parralell assingment, of all the
pipe stages and the outputs, unless it adds extra latches for the
outputs !

Whereas the original way with = for the pipe parts, it can see that the
last assignment to the output is just a copy of the last pipe stage and
so dosn't require a sepearte latch.

Allthough, that's still too woolly for my likeing. I'd like the code to
be obvious, and garenteed to produce the number of pipe stages I've
specified in DEPTH.

Chris
 
On 2011-04-21 23:19:30 +0100, Chris Hinsley said:

I can just about see why it might do that compared to the previous
code, but it's not obvious.

All good learning fun this ! :)

Chris

My thoughts are that it can't do a parralell assingment, of all the
pipe stages and the outputs, unless it adds extra latches for the
outputs !

Whereas the original way with = for the pipe parts, it can see that the
last assignment to the output is just a copy of the last pipe stage and
so dosn't require a sepearte latch.

Allthough, that's still too woolly for my likeing. I'd like the code to
be obvious, and garenteed to produce the number of pipe stages I've
specified in DEPTH.

Chris
How about this ? It produces the correct pipeline stages, has all <=,
but I'm not keen on the (DEPTH - 2), but apart from that ?

always @(posedge i_clk or negedge i_rst)
begin
reg [(DEPTH - 2):0] [(REG_WIDTH - 1):0] pipe_wb;
reg [(DEPTH - 2):0] [(REG_SEL_BITS - 1):0] pipe_wbr;

if (!i_rst)
begin
pipe_wb <= 0;
pipe_wbr <= 0;
o_wb <= 0;
o_wbr <= 0;
end
else
begin
integer i;
for (i = 0; i < DEPTH; i++)
begin
if (i == 0)
begin
o_wb <= pipe_wb;
o_wbr <= pipe_wbr;
end
else if (i == (DEPTH - 1))
begin
pipe_wb[i - 1] <= wb;
pipe_wbr[i - 1] <= wbr;
end
else
begin
pipe_wb[i - 1] <= pipe_wb;
pipe_wbr[i - 1] <= pipe_wbr;
end
end
end
end


Chris
 
In article <2011042123353443426-chrishinsley@gmailcom>,
Chris Hinsley <chris.hinsley@gmail.com> wrote:
How about this ? It produces the correct pipeline stages, has all <=,
but I'm not keen on the (DEPTH - 2), but apart from that ?

snip example code

Chris, I've not followed your example too closely, and Jonathan's
got you on track better than anyone else could. But I'll offer up
what we do to model a "pipeline" delay.

We've got a very simple parameterizable module:

module pipe_dly
#(
parameter WIDTH = 8,
parameter DEPTH = 3
)
(
input wire clk_i,
input wire en_i,
input wire [ WIDTH - 1 : 0 ] d_i,
output wire [ WIDTH - 1 : 0 ] q_o
);

The basic guts:
//reg [ DEPTH - 1 :0 ] [ WIDTH - 1 : 0 ] shift_regs;
reg [ DEPTH * WIDTH - 1 : 0 ] shift_regs;

The first (commented line) is how we'd like to declare our
register. But Xilinx doesn't support SystemVerilog. Makes
no difference to the functional code, in this case. If your
tools support SystemVerilog, then I'd use the commented case.

always @( posedge clk_i )
if( en_i )
shift_regs <= { shift_regs, d_i };

assign q_o = shift_regs[ ( DEPTH - 1 ) * WIDTH +: WIDTH ];

That's basically it. We've got a generate loop to take
case of the special case of DEPTH = 0 (assign q = d).

This logic will map to the efficient SRL16 structures in Xilinx
arch. Perfect for pipeline stages. As I said, I didn't look
too close at what you're trying to do. But we get a LOT of mileage
out of this little module.

--Mark
 
On Thu, 21 Apr 2011 23:12:17 +0100, Chris Hinsley wrote:

Agreed. As a matter of interest, could you try it with all
the assignments to pipe_* changed to be <= ? I don't think
that would alter the functionality in this case

But, interesting result if I do put it all to <=, Quartus dosn't
complain, but it (at the RTL level) has the output as an extra stage in
the pipeline !!!
Just a very quick response because it's a nice vacation day
and I'm busy in the garden, but... my bad. When I said

I don't think that would alter the functionality
what I really meant was "I didn't think". For pipe[0],
and only for pipe[0], you're reading the variable immediately
after you assigned to it. If you do blocking assignment to
pipe[0] then the immediately subsequent read (in the same
pass through the code, at the same clock edge) will pick
up the newly assigned value - the value that is about to be
saved in pipe[0] by the clock - and so the output is a clone
of pipe[0]. But if you do nonblocking <= assignment to
pipe[0] and then read it, the read picks up the old,
pre-clock value - the value that was stored into pipe[0]
on the previous clock - and so you get an additional stage.

Apologies for misleading you. Interesting, though, that
Quartus has this restriction on assignment to output variables
outwith the clock/reset "if" block.

I can just about see why it might do that compared to the
previous code, but it's not obvious.
Well, it was obvious to the synthesis tool and it *should* have
been obvious to me, but I agree that it's tricky to get your
head around it when coming to it for the first time.

All good learning fun this ! :)
Yup. I'll try to say a bit more, and respond also to Mark's
very useful post, later today.

cheers
--
Jonathan Bromley
 
Chris Hinsley wrote:
Folks, is this a reasonable and correct way to express a pipeline ? I'm
deliberating on things like the use of = or <= in the loop. Advise would
be welcome.

[snip]
for (i = 0; i < DEPTH; i++)
I've always written this as
for (i = 0; i < DEPTH; i = i + 1)
because sometime back (Verilog 95) I found that i++ didn't work. I
certainly use that in C, but not Verilog. Did the autoincrement syntax
get updated with Verilog 2001? Or is this a System Verilog addition?

-- Gabor
 
On 2011-04-22 15:51:46 +0100, Gabor said:

Chris Hinsley wrote:
Folks, is this a reasonable and correct way to express a pipeline ? I'm
deliberating on things like the use of = or <= in the loop. Advise
would be welcome.

[snip]
for (i = 0; i < DEPTH; i++)
I've always written this as
for (i = 0; i < DEPTH; i = i + 1)
because sometime back (Verilog 95) I found that i++ didn't work. I
certainly use that in C, but not Verilog. Did the autoincrement syntax
get updated with Verilog 2001? Or is this a System Verilog addition?

-- Gabor
I just naturally did i++, coming from C++. It means exactly the same thing ?

Chris
 
always @( posedge clk_i )
if( en_i )
shift_regs <= { shift_regs, d_i };

assign q_o = shift_regs[ ( DEPTH - 1 ) * WIDTH +: WIDTH ];

That's basically it. We've got a generate loop to take
case of the special case of DEPTH = 0 (assign q = d).
While I like the simplicity of expressing this in one line, I don't
quite like it.

Are you relying on the compiler to just discard the top part of the
composite value ?

That might be OK, but again, I'm not keen on trusting the compilers to
do this correctly. Is this a garenteed action ?

Chris
 
On Fri, 22 Apr 2011 16:47:23 +0100, Chris Hinsley wrote:

I just naturally did i++, coming from C++. It means exactly the same thing ?
It was added in SystemVerilog, and has the same meaning as in C.

I believe that some tools still don't support the use of ++/--
in an expression (you know, the usual a[i++] thing). But they
are fine as simple increment operators, equivalent to +=1 (also
supported in SystemVerilog but not in classic Verilog).

The syntax of Verilog bears many superficial resemblances to
C, but it's foolhardy to extrapolate without checking the
consequences. The existence of X values and arbitrary
bit-width expressions makes things distinctly different
in Verilog, and there are other pinch-points too.
--
Jonathan Bromley
 

Welcome to EDABoard.com

Sponsor

Back
Top