XLS should codegen explicitly-sized sub-expressions to avoid self-determined widths
- Dominant language
- C++
- Stars
- 1.9k
- Forks
- 283
- Avg merge
- 2d 10h
- Merged PRs (30d)
- 135
Description
### What's hard to do? (limit 100 words)
XLS generates expressions relying on implicitly sized bitwidths according to LRM Table 11-21. In general, for hardware design, this is problematic because the bitlengths of self-determined expressions can be counterintuitive. Examples:
* `a * b` and `a + b`, etc. produces a result of length `max($bits(a), $bits(b))`. `2'b10 + 2'b10` as a subexpression produces a result of `2'b00`.
* In `logic [7:0] foo; logic bar; logic baz; assign foo = {(bar << 3), (baz << 3)};`, `bar << 3` has length 1 (`$bits(bar)`), and `foo` evaluates to zero.
Due to these types of bugs that can be quite hard to detect in DV, chip projects commonly have lint rules in place to flag self-determined bitlengths in arithmetic expressions that may not produce the intended result. The Google Verilog style guide specifies avoiding these cases by either:
* Casting the sub-expression to a specific width
* Breaking down compound expressions into separate intermediate variables.
XLS knows the bitwidths of everything so should actually do the right thing, but generated code can still hit these lint errors. For example,
https://github.com/google/xls/blob/a69abccc21ece6e734ae8ee6ab3db4c5bdc545e5/xls/dslx/stdlib/apfloat.x#L414-L421
results in something including:
`'23'h7f_ffff << fltirst_lost_bit_idx__1`
where `fltirst_lost_bit_idx__1` is 32 bits, so the lint tool thinks you might expect a larger result. In actuality, we know that the bit index is something in range, and we still expect a 23 bit result.
### Current best alternative workaround (limit 100 words)
We can globally waive this type of lint error for an XLS generated file.
### Your view of the "best case XLS enhancement" (limit 100 words)
Ideally, XLS generated RTL should be lint clean when reasonable and follow established best style practices.
Contributor guide
Assessment
This issue has not been assessed yet.