PeakRDL-regblock icon indicating copy to clipboard operation
PeakRDL-regblock copied to clipboard

Saturating counters perform inequality check twice in generated logic

Open paul-demo opened this issue 8 months ago • 0 comments

For this counter description:

signal {
    sync;
    activehigh;
} latch_stats;

reg stats_cnt_reg_varincr #(
    string regdesc = "",
    string fielddesc = "",
    longint regwidth = 32,
    longint fieldwidth = 32,
    longint incrwidth = 11,
    boolean saturate = true
) {
    desc = regdesc;
    regwidth = regwidth;
    buffer_reads = true;
    rbuffer_trigger = latch_stats;

    field {
        desc = fielddesc;
        hwclr = latch_stats;
        sw = r;
        hw = na;
        counter;
        incrwidth = incrwidth;
        saturate = saturate;
    } cnt[fieldwidth-1:0];
};

regfile some_regfile {
    stats_cnt_reg_varincr #(
        .fieldwidth(16)
    ) bytes_dropped @ 0x38;
}

PeakRDL-regblock geneates this block of code:

    // Field: eth_ingress_csr.glbl.bytes_dropped.cnt
    always_comb begin
        automatic logic [15:0] next_c;
        automatic logic load_next_c;
        next_c = field_storage.glbl.bytes_dropped.cnt.value;
        load_next_c = '0;
        if(latch_stats) begin // HW Clear
            next_c = '0;
            load_next_c = '1;
        end
        if(hwif_in.glbl.bytes_dropped.cnt.incr) begin // increment
            if(((17)'(next_c) + hwif_in.glbl.bytes_dropped.cnt.incrvalue) > 16'hffff) begin // up-counter saturated
                next_c = 16'hffff;
            end else begin
                next_c = next_c + hwif_in.glbl.bytes_dropped.cnt.incrvalue;
            end
            load_next_c = '1;
        end
        field_combo.glbl.bytes_dropped.cnt.incrthreshold = (field_storage.glbl.bytes_dropped.cnt.value >= 16'hffff);
        field_combo.glbl.bytes_dropped.cnt.incrsaturate = (field_storage.glbl.bytes_dropped.cnt.value >= 16'hffff);
        if(next_c > 16'hffff) begin
            next_c = 16'hffff;
            load_next_c = '1;
        end
        field_combo.glbl.bytes_dropped.cnt.next = next_c;
        field_combo.glbl.bytes_dropped.cnt.load_next = load_next_c;
    end

The comparison if(next_c > 16'hffff) is unnecessary. It's tautological too, so maybe synthesis tools would be able to optimize it away (ie, a 16 bit value can never be larger than 16'hffff).

This throws a linter error in Verilator which says something like "this comparison will always be false". I'm imagining the generated code is an artifact of generalized overflow checking, if for example the limit was less than 2**N-1 and we had some other way to update the value like sw = w or hw = w. But, as it is now, the sum is already checked for overflow about 9-10 lines above, and then it's checked a second time.

What do you think? If you think synthesis tools would be able to optimize this away, then I guess no harm in leaving it. But I wanted to bring it up since those inequality comparisons can get expensive for saturating logic, particularly with wider fields, so it's desirable not to do all of them twice!

paul-demo avatar Jun 07 '24 21:06 paul-demo