Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add vpiTimeUnit and allow to specify time as string #1636

Closed
veripoolbot opened this issue Dec 12, 2019 · 5 comments
Closed

Add vpiTimeUnit and allow to specify time as string #1636

veripoolbot opened this issue Dec 12, 2019 · 5 comments
Labels
area: usability Issue involves general usability resolution: fixed Closed; fixed

Comments

@veripoolbot
Copy link
Contributor


Author Name: Stefan Wallentowitz (@wallento)
Original Redmine Issue: 1636 from https://www.veripool.org

Original Assignee: Stefan Wallentowitz (@wallento)


Add vpiTimeUnit, despite there is not much value in it for Verilator, but for completeness.

Allow to specify the time unit und the precision to be specified as strings (VL_TIME_PRECISICON_STR and VL_TIME_UNIT_STR), ranging from 1fs to 100s. This makes it compatible with some simulators that use -timescale on the command line in that format.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-12-12T14:15:43Z


https://github.com/wallento/verilator/tree/issue-1636

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-13T12:16:31Z


Seems reasonable. The real fix which might replace some of this is to add correct time support. This is on the roadmap, #�, and something would be great to get in if you/someone has a good time chunk available.

+++ b/include/verilated.h
@@ -737,9 +737,45 @@ extern void _VL_DEBUG_PRINT_W(int lbits, WDataInP iwp);
+// Helper function for conversion of timescale strings
+// Converts (1|10|100)(s|ms|us|ns|ps|fs) to power of then
+static inline int VL_TIME_STR_CONVERT(const char *str) {

"const char* strp".

I doubt this function will ever be hot, so just extern it in verilated.h and move imp to verilated.cpp.

+    int scale = 0;
+    ...
+    case 'm': scale -= 3; break;

Think you can just use "scale = 3". etc.

+#define VL_STRINGIFY(x) VL_STRINGIFY2(x)
+#define VL_STRINGIFY2(x) #x

With these, you'll need to remove them from several test*/.cpp files to avoid redefinitions. These are also generically useful, let's please move them to verilatedos.h.

 #ifndef VL_TIME_PRECISION
+#ifdef VL_TIME_PRECISION_STR
+# define VL_TIME_PRECISION VL_TIME_STR_CONVERT(VL_STRINGIFY(VL_TIME_PRECISION_STR))
+#else
 # define VL_TIME_PRECISION (-12)  ///< Timescale units only for for VPI return - picoseconds
 #endif

Please add spaces needed for consistent indent.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-12-13T20:05:41Z


Hi,

I have amended the branch and split into three commits. First is simply adding the VL_STRINGIFY. I intentionally added the VL_ in front to not have it conflict, but every repeating code is bad, so I added the second commit to replace the STRINGIFY macros in the tests whereever possible, those that are used as PLI don't include the Verilator infrastructure and I am generally not sure how it would work with other simulators, or isn't that the intention? Sorry, I only used the tests with Verilator but understand the intention is to also test with other tools, correct?

Third commit is the main part amended with your suggestions. It has to be @scale -= ..@ because for 10 and 100 we count up before (@while (*str == '0') { scale++; str++; }@).

See https://github.com/wallento/verilator/tree/issue-1636

You can assign #234 to me, but I cannot commit to a timeline, but hopefully not another 9 years.

Best,
Stefan

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-14T00:28:27Z


Great, pushed to git towards eventual 4.026 release.

@veripoolbot veripoolbot added area: usability Issue involves general usability reolution: fixed resolution: fixed Closed; fixed and removed reolution: fixed labels Dec 22, 2019
@wsnyder
Copy link
Member

wsnyder commented Dec 22, 2019

Now closing issues on git, versus release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: usability Issue involves general usability resolution: fixed Closed; fixed
Projects
None yet
Development

No branches or pull requests

2 participants