Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Saturating counters perform inequality check twice in generated logic #114

Open
paul-demo opened this issue Jun 7, 2024 · 0 comments
Open

Comments

@paul-demo
Copy link

paul-demo commented Jun 7, 2024

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant