odb: fix wire decoding for the subsequent segment of a kColinear junction with extensions#9765
Conversation
…tion with extensions Signed-off-by: Arthur Koucher <arthurkoucher@precisioninno.com>
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug in the wire decoding logic for kColinear junctions with extensions. The change ensures the extension value is read from the correct index. A new test case has been added to cover this specific scenario, which is great. My review focuses on a maintainability improvement: the fix introduces duplicated code in two overloads of the getSegment function. I've suggested refactoring this duplicated logic to improve code quality, aligning with the principle of encapsulating logic in helper functions to avoid duplication.
| const WireOp wire_op = static_cast<WireOp>(opcode & WOP_OPCODE_MASK); | ||
| cur_ext = wire_op == kColinear ? wire->data_[idx] : wire->data_[idx + 1]; |
There was a problem hiding this comment.
This logic is duplicated in the getSegment overload at lines 790-791. To improve maintainability and avoid code duplication, consider extracting this logic into a static helper function. This aligns with the principle of encapsulating logic in helper functions to avoid duplication.
For example:
static int getCurExt(const _dbWire* wire, unsigned char opcode, int idx)
{
const WireOp wire_op = static_cast<WireOp>(opcode & WOP_OPCODE_MASK);
return wire_op == kColinear ? wire->data_[idx] : wire->data_[idx + 1];
}This would allow you to call cur_ext = getCurExt(wire, opcode, idx); in both locations.
As a smaller change, this logic can be condensed into a single line.
cur_ext = (static_cast<WireOp>(opcode & WOP_OPCODE_MASK) == kColinear) ? wire->data_[idx] : wire->data_[idx + 1];References
- Encapsulate the logic for calculating master symmetry in a helper function to avoid code duplication.
There was a problem hiding this comment.
I think that conceptually it would be better to refactor the entire state machine as there are two large blocks of code for it. That said, it would be make the review rather painful if included in this PR.
|
clang-tidy review says "All clean, LGTM! 👍" |
Related to #3969.
Context
While using the new segment regression from #3989 to generate new
set_layer_rcvalues for asap7, I stumbled into a behavior in which, only onM2, the regression coefficient of determination was very poor for resistance.This didn't seem to make any sense, because, based on the fact that the RCX extraction model computes resistance as
And for minimum-width segments width is constant and we'd have:
The result is literally a linear relation, so we should have
R² == 1- which we do for all the other layers.After some debugging I realized that in the generated RC
.csvthere was a couple of very large segments (literally the size of the entire core width) of a certain net with a much lower per-length resistance than the rest of the segments. After using the GUI to take a look at the net it became clear that the segments' shapes were being wrongly computed as it was a net with relatively short segments.These problematic segments have a rare type of
WireOpjunction. Basically, AFAIU, a junction with forward and backward extensions can be encoded in two ways:dbWireEncoder::newPath+addPointwith extension):kJunctionjct_idkX | WOP_EXTENSIONx_coordkOperandextension← read fromdata_[idx+1]dbWireEncoder::newPathExt):kJunctionjct_idkColinear | WOP_EXTENSIONextension← read fromdata_[idx]kXx_coordThe second one is rarer (this is still a bit of a mystery to me, maybe @osamahammad21 knows?) and only seems to appear in a very small amount of segments even in designs with many nets.
Problem
This rare case is not covered by the decoder state machine in
dbWire::getSegment. What happens is that the decoder ends up treating the junction as it were a "normal branch" and thinks that the extension value must be retrieved from the subsequent index (data_[idx + 1]). However, the extension value for a kColinear junction is stored in its own index (data_[idx]).Subsequently the function that actually computes the geometry receives a wrong extension value and the final shape gets a completely wrong geometry.
Fix
When decoding and stumbling into a kColinear junction, retrieve the extension value correctly.