RTL coding quality and design review

Explanation

Review in layers

Start with the requirement before reading syntax:

1. Interface: signal meaning, direction, width, signedness, clock domain, and handshake.
2. State: registers, reset values, enables, legal transitions, and recovery.
3. Datapath: numeric range, extension, truncation, overflow, latency, and throughput.
4. Combinational logic: complete assignment, priority, defaults, and loops.
5. Structure: hierarchy, parameters, generated variants, memories, and ownership.
6. Evidence: lint, compilation, simulation checks, synthesis messages, and reviewed waivers.

High-value defect patterns

Look for:

• implicit or misspelled nets;
• width mismatch and unsized literals;
• mixed signed and unsigned arithmetic;
• blocking assignment in sequential state logic;
• nonblocking assignment used as ordinary combinational calculation;
• incomplete combinational assignment and inferred latches;
• multiple drivers;
• ordinary logic used as a clock;
• reset polarity or priority mismatch;
• unreachable or unrecoverable FSM states;
• simulation-only constructs in RTL;
• out-of-range array access;
• undiagnosed x values;
• output protocols that can deadlock or drop data.

Warnings and waivers

Aim for a clean, agreed warning baseline. Do not suppress a category merely because one message is inconvenient. A waiver should name the exact instance or source, rule, reason, reviewer, owner, and expiry or recheck condition.

Simulation and synthesis agreement

Synthesis supports a defined RTL subset and may optimize unreachable or constant behavior. Review tool messages for removed registers, undriven signals, inferred latches, unexpected memories, and unconnected ports. Run a post-synthesis structural check when the project flow requires it, but keep readable RTL and strong pre-synthesis tests as the main prevention method.

Match each report to the question it can answer

Engineering reports are useful only when their scope is clear. A green status from one tool does not replace the evidence produced by another.

For each result, record the tool and version, command, source revision, test scope, pass/fail outcome, and unresolved limitations. A useful closure statement names the requirements supported by the evidence and the cases that remain untested.

X-state discipline

An x may expose missing initialization or illegal behavior. It can also be masked by optimistic conditional semantics. Make reset and illegal-state policy explicit, use strict testbench comparisons, and investigate the first source of an unknown rather than its final visible symptom.

Definition of done for a lesson project

• Requirement and assumptions are written.
• RTL compiles with the approved warning set.
• Lint has no unresolved high-severity findings.
• A self-checking test covers reset, nominal, boundary, and error behavior.
• The transcript has explicit pass/fail status.
• Waveform dumping can be enabled for diagnosis.
• Synthesis accepts the RTL and inferred structures match intent.
• Another engineer can reproduce the run from documented commands.

Worked example

A reviewer sees sum = a + b; where all three signals are eight bits and the requirement calls for a carry flag. The syntax is legal, but the design loses overflow information. The repair widens both operands and the intermediate result, then assigns low data bits and carry separately. Tests must include 8'hFF + 8'h01.

Defective version: carry is lost

module add8_bad (
    input  wire [7:0] a,
    input  wire [7:0] b,
    output wire [7:0] sum,
    output wire       carry
);
    assign sum   = a + b;
    assign carry = 1'b0;  // Incorrect: overflow is discarded.
endmodule

Reviewed version: widen before adding

module add8_checked (
    input  wire [7:0] a,
    input  wire [7:0] b,
    output wire [7:0] sum,
    output wire       carry
);
    wire [8:0] extended_sum;

    assign extended_sum = {1'b0, a} + {1'b0, b};
    assign {carry, sum} = extended_sum;
endmodule

The expression is explicitly nine bits because each operand is extended before the addition. For a = 8'hFF and b = 8'h01, the expected result is carry = 1'b1 and sum = 8'h00.

Compile with warnings enabled and keep the transcript:

iverilog -g2012 -Wall -o simv add8_checked.v tb_add8_checked.v
vvp simv | tee add8_checked.log

Check yourself

Why should review begin with interface and numeric intent? What evidence justifies a lint waiver? Which test exposes an eight-bit addition that accidentally discards carry?