verilog: indirect AST_CONCAT and AST_TO_UNSIGNED port connections #4201
Conversation
| lookup_suggested = true; | ||
| else if (value->type == AST_CONCAT && value->children.size() == 1) | ||
| // concat of a single expression is equivalent to $unsigned | ||
| lookup_suggested = true; |
There was a problem hiding this comment.
OK, so suggesting a lookup here will make us indirect all port connections that are not connections to a plain identifier.
There was a problem hiding this comment.
Correct. Is this problematic? Should I clarify the surrounding code?
There was a problem hiding this comment.
Well, the intent wasn't really obvious the first moment I looked at this if (type == AST_CELL) { branch, so any clarification would be nice. Though I wouldn't consider it required for the PR.
| else | ||
| // unless this instantiation depends on module | ||
| // information that isn't available yet | ||
| if (!attributes.count(ID::reprocess_after)) | ||
| log_assert(arg->is_signed == sig.as_wire()->is_signed); |
There was a problem hiding this comment.
So what we are checking here is: does the wire we are connecting have an is_signed flag matching the signedness of the expression in the connection?
There was a problem hiding this comment.
That's correct. Should I clarify this comment?
There was a problem hiding this comment.
No need I think, I am primarily checking my understanding of what happens here, and maybe leaving some notes for anyone else who would look at the PR.
| assign out = v[0]; | ||
| logic [3:0] v[1:0]; | ||
| producer p0(v[0]); | ||
| producer p1({v[1]}); |
There was a problem hiding this comment.
I am not clear on how the code change interacts with this example. We seem to be addressing signedness mismatches, but there's not a signed variable in sight here.
There was a problem hiding this comment.
Ah, I think this has to do with the fact that memory writes need to be indirected, for reasons unrelated to signedness, but if wrapped in an AST_CONCAT they did not trigger the indirection previously.
There was a problem hiding this comment.
But if that's the case, shouldn't we trigger the indirection in all cases of AST_CONCAT, not only those with children.size() == 1?
There was a problem hiding this comment.
You're right! I'll address this.
- AST_CONCAT and AST_TO_UNSIGNED are always unsigned, but may generate RTLIL that exclusively reference a signed wire. - AST_CONCAT may also contain a memory write.
Fixes #4158.