Fix write_verilog escape seq Issue 3826

[openroad-ci]

No description provided.

[dsengupta0628]

Fix for The-OpenROAD-Project/OpenROAD-flow-scripts#3826

There are 2 src file changes and one testcase added in test/ directory to verify the golden output (.ok file), and one output is regoldened:

src/sta/verilog/VerilogWriter.cc
VerilogWriter::writeInstBusPin was using the portname directly from the Network object instead of converting to a verilog name. So we need to write this string port_vname = portVerilogName(network_->name(port)) instead of network_->name(port) in the output verilog. This fixed the issue when the portname was getting the extra escapes like Y[32:1] instead of Y[32:1] in the Verilog that was written out by write_verilog (read_verilog shows error when encountering Y[32:1] which is now fixed)

src/sta/network/VerilogNamespace.cc
staToVerilog function that converts the sta names to Verilog names was keeping double escape sequence as is. So for example, a hierarchical cell \a\x in original verilog produced by Yosys, would mistakenly get written out as a\x by write_verilog (read_verilog shows error when encountering a\x which is now fixed). The bug in this function is due to an inconsistency with the verilogToSta function that adds escape to any existing escape sequence. So unescaping logic wasn't consistent, i.e., not removing the extra "". I have also updated the staToVerilog2 function accordingly for completion.

We also need to regolden the following test's expected putput in OpenROAD regression suite src/dbSta/test/hierwrite.tcl
(src/dbSta/test/hierwrite.vok)
This file was expecting Verilog names with "$" signs to be allowed. It was very inconsistent in escaping and nonescaping words with $ sign. Even if read_verilog would pass, a non-escaped word containing "$" is illegal in Verilog. Fixed this .vok file and this regression should pass: hierwrite.tcl

Details of fix in src/sta/network/VerilogNamespace.cc explaining the unescaping logic and the small bug I fixed there.

The escaping logic was implemented in verilogToSta function. If any name in the Verilog input began with a "" it is considered an escaped name. In such a name, if either of "[" or "]" or "/" or "" is encountered, an extra "" is added and the leading "" (meaning the one on the front of the name) is skipped when storing as sta_name that is understood by code.

There was a bug in the "unescaping" logic.

Now when we convert from sta_name to a verilog name via staToVerilog or staToVerilog2 (to write into Verilog output), it should be inverse of the verilogToSta, i.e., if we encounter any "" in the name, it means it was escaped, so needs to be unescaped before writing as a verilog name.
Note that Read_verilog would have failed if user had a name like h1\x in the original Verilog instead of \h1\x. So anytime we encounter "" in staToVerilog or staToVerilog2, it MUST have been escaped by verilogToSta function

Unescaping logic is as follows

if any character in sta_name was "", meaning it was escaped, and the following needs to be done :
(1) we must set "escaped" to true. --> This was incomplete because when even if next char was "" escaped must be true
(2) if character after "" was also another "", we should skip double escape and just write a single "",--> This was a fix as previously double escaping was done causing read_verilog failure
(3) if character after "" was not another "", then we don't do anything with current "" and proceed with next character, so if sta_name was like a/x, when looking at "", next character is "/", so we skip this iteration of the loop and don't write "", and in next iter just go into the else block of if (ch == verilog_escape) block. This is where there is slight difference in staToVerilog and staToVerilog2 though decision is almost the same
staToVerilog sees current character as "/" in the else block and it is neither alphanumeric nor "_", so marks as escaped=true and writes this character as is. This is called for module and instance names.
staToVerilog2 (that is called for bus and ports), sees current character as "/" in the else block which satisfies this complicated looking if condition, so marks escaped as true and writes this character "/": The big condition is basically saying that "ch" is considered escaped if it is either a square bracket, or it is neither a square bracket nor alphanumeric nor "". So a "[" or "]" or "$" or any special character other than "" would result in escaped=true.
And if the character was escaped, and extra space is added- I think this was to fix an old issue.
If character was not escaped, then the original sta_name is written out in the verilog.

Ideally, we could just keep the logic of staToVerilog2 since it is a superset of staToVerilog but that would a bigger change.

[jhkim-pii]

@vvbandeira PR-merge CI failed due to the OR regression fail. In this case, I think it is better to mark the CI as PASS because we can see the FAIL sign after bumping the OpenSTA version in OR.

[maliberty]

@vvbandeira is the OR CI running with version of the sta submodule? If so, then the failure would be of interest to this PR.

[dsengupta0628]

@vvbandeira is the OR CI running with version of the sta submodule? If so, then the failure would be of interest to this PR.

Hi @maliberty yes the OR CI is running with the sta submodule, and this failure is related to my change- I debugged in detail too.

The regression had a portname like this .in_$000({req_msg_a[15] in the expected verilog from write_verilog .
But the "$" sign in a Verilog port name should be escaped if the VerilogWriter.cc did not have the mistake that I fixed.
So updating from
fprintf(stream_, ".%s({", network_->name(port));
to
string port_vname = portVerilogName(network_->name(port));
fprintf(stream_, ".%s({", port_vname.c_str());

caused the change that now the output verilog will have this $ sign in portname also escaped: .\in_$000 ({req_msg_a[15],
(even if I don't do any other changes in the VerilogNamespace)

So I think we needed to regolden the regression as this was a legit mistake in OpenSTA.
After this checkin, I will have to regolden the OR regression- already have a branch for that.

Please note, even though both .in_$00 and .\in_$00 are readable portnames by read_verilog, an unescaped $ in verilog name is technically illegal so the latter should be correct.

[maliberty]

Ok so long as we know that it just an .ok update to OR. I plan to merge #288 first as it is quite large and I want to avoid merge conflicts in it.

[vvbandeira]

@jhkim-pii
The idea is that the OR Regressions would work as an early warning system. For example, if the test fails, you have the opportunity to evaluate if the error makes sense or if it should not have happened.

I plan to split the pr-merge report into multiple checks to make it clearer.

@maliberty
Yes, the OR regressions are using the commit from the current PR/branch.