[ci] Add slang lint#445
Conversation
There was a problem hiding this comment.
Hello! I think automatic variables here will make waveform track difficult since they will not be shown as a signal
There was a problem hiding this comment.
Hi! here, with automatic prefix we will not be able to track signal r when debugging waveform
There was a problem hiding this comment.
For "VSSUB" assignment (line 312, 318, 324), the functionality is changed:
old code: res.w16[b] = vxsat.w16[b] ? (opb.w16[b][15] ? 16'h8000 : 16'h7FFF) : sub[15:0];
new code: res.w16[b] = &vxsat.w16[b] ? (opb.w16[b][15] ? 16'h8000 : 16'h7FFF) : sub[15:0];
So previously, as long as 1 bit is 1, the boolean is true, the new code needs all vxsat.w16[b] bits to be high.
Maybe we should change it to "res.w16[b] = |vxsat.w16[b] ? (opb.w16[b][15] ? 16'h8000 : 16'h7FFF) : sub[15:0];"
Similar changes apply to "res.w32[b]" and "res.w64[b]" in VSSUB block.
There was a problem hiding this comment.
So, if I'm not mistaken, the old implementation does not consider all bits of vxsat.w16[b]. The newer implementation takes into account all bits and mirrors the functionality in VSSUBU directly above.
There was a problem hiding this comment.
In VSSUBU (EW16 for example), they have "vxsat.w16[b] = {2{sub[16]}};", so they replicate the "sub[16]" single bit to all bits in "vxsat.w16[b]", but in VSSUB, this bit is not replicated to all bits, so they only need to check the lowest bis, this is why "&" would be wrong here.
There was a problem hiding this comment.
To me, this change could reduce the readability of the code
There was a problem hiding this comment.
Automatic prefix will make debugging hard, they do not exist as named signals in the design hierarchy
There was a problem hiding this comment.
Automatic prefix will make debugging hard, they do not exist as named signals in the design hierarchy
There was a problem hiding this comment.
Automatic prefix will make debugging hard, they do not exist as named signals in the design hierarchy
There was a problem hiding this comment.
Automatic prefix will make debugging hard, they do not exist as named signals in the design hierarchy
There was a problem hiding this comment.
This changes the functionality, I need to understand the code further
There was a problem hiding this comment.
AFAIK this is not a functional change, only a clarification: >> has lower precedence than -; - will be evaluated first
PR review (hopang-0221): the automatic variables introduced to silence slang's -Winferred-latch are not visible in waveforms, since they do not exist as named signals in the design hierarchy. Restore them as module-scope signals and break the inferred latches with unconditional default assignments at the top of their always_comb blocks instead. No functional change: each of these is always assigned before it is read on the path that uses it, so the defaults only affect the otherwise-unread latch paths. Affected: simd_alu (r), lane_sequencer (extra_stride/vl_tot), masku (vrgat_res/vrgat_buf), vldu (vrf_word_start_byte), vstu (vrf_eff_write_bytes), vmfpu (vfrec7/vfrsqrt7 scratch arrays, narrowing_shuffled_result/narrowing_shuffle_be). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR review (hopang-0221): unlike VSSUBU, the VSSUB saturation flag is not
replicated across all bits of vxsat.w{16,32,64}[b] (only the LSB is set),
so the AND-reduction introduced for the slang -Wint-bool-conv fix is
always 0 and saturation never triggers. Use OR-reduction instead, which
matches the original non-zero test and clears the warning.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR review (hopang-0221): replace the verbose explicit zero-pad
replications (added to avoid an unsized '0 inside a concatenation) with
equivalent implicit zero-extension and a plain literal comparison.
out_pnt_d is wide enough to hold {red_stride_cnt_d, 3'b0}, and the popc
compare is against the value 1 - no functional change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
No description provided.