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

Complete string methods (starter project) #1606

Closed
veripoolbot opened this issue Nov 17, 2019 · 12 comments
Closed

Complete string methods (starter project) #1606

veripoolbot opened this issue Nov 17, 2019 · 12 comments

Comments

@veripoolbot
Copy link
Contributor

@veripoolbot veripoolbot commented Nov 17, 2019


Author Name: Wilson Snyder (@wsnyder)
Original Redmine Issue: 1606 from https://www.veripool.org


Implement the built-in string methods that are remaining:

  • atobin, atohex, atoi, atooct, atoreal: Become a scanf.

  • compare, icompare: Become new AstCompareNN, AstCompareINN.

  • getc, putc: Perhaps new AstGetCN/ AstPutCN, maybe map onto something existing?

This would be a good starter project for someone.

Test exists in t_string_type_methods.v.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 7, 2019


Original Redmine Comment
Author Name: Yutetsu TAKATSUKASA (@yTakatsukasa)
Original Date: 2019-12-07T15:06:34Z


Hi
Can I tackle this item ?

I just added compare and icompare support here.
https://github.com/yTakatsukasa/verilator/commits/string_methods_1606

Yes. This task is a good starting point to learn AstNode.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 7, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-07T16:27:57Z


Great job! Here's some comments on the patch:

Please add your name to docs/CONTRIBUTORS to acknowledge the contribution as explained in that file.

+ static inline IData VL_CMP_NN(const std::string &lhs, const std::string &rhs) {
+ virtual void visit(AstCompareNN *nodep) { visit_compare_string(nodep); }
+ AstNode *newp = new AstCompareNN(nodep->fileline(), lhs, rhs);
+ ... others

Nit, put the * or & attached to the type, e.g. "std::string& ", "AstNode* ".

+++ b/include/verilated_heavy.h
+    const int ret = std::strcmp(lhs.c_str(), rhs.c_str());
+++ b/src/V3Number.cpp
+    const int result = std::strcmp(lhs.toString().c_str(), rhs.toString().c_str());

Add a comment above these that Verilog doesn't allow \0 in strings (so we don't have to use C++'s compare.)

+++ b/src/V3Width.cpp
+            if (case_insensitive) {  // capitalize both strings to ignore case
+                lhs = new AstToUpperN(nodep->fileline(), lhs);
+                rhs = new AstToUpperN(nodep->fileline(), rhs);

Converting is a good hack, but is slower as requires (potentially) two new temporaries versus runtime non-modifying code. I'd suggest add a bool to AstCompareNN to indicate case-insensitve and pass that down. (Another alternative would be another new Ast type, but that seems overkill.)

You'd then call strcasecmp, but that's not in windows, so add to verilatedos.h

    #ifdef _MSC_VER
    # define VL_STRCASECMP _stricmp
    #else
    # define VL_STRCASECMP strcasecmp
    #endif

+++ b/src/V3Width.cpp
+    virtual void visit(AstCompareNN *nodep) { visit_compare_string(nodep); }
...
+    void visit_compare_string(AstNodeBiop* nodep) {

No need for the extra function up top as this is called just once, use

     virtual void visit(AstCompareNN *nodep) {
         ...
@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 8, 2019


Original Redmine Comment
Author Name: Yutetsu TAKATSUKASA (@yTakatsukasa)
Original Date: 2019-12-08T03:18:39Z


Thanks for the feedback.

I have updated the branch as you suggested.

Travis look fine with the change. https://github.com/yTakatsukasa/verilator/runs/338459006

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 8, 2019


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


Very good again.

 +++ b/include/verilated.cpp
 +#include <algorithm>
  #include <cctype>
 +#include <cerrno>

Adding these will slow down compiles, so please move it to verilated_heavy.cpp, then update the needHeavy check to require heavy if ATOI is present.

 +++ b/include/verilated_heavy.h
 +inline IData VL_CMP_NN(const std::string& lhs, const std::string& rhs, bool ignoreCase) VL_PURE {
 +    // SystemVerilog Language Standard does not allow a string variable to contain '\0'.
 +    // So C functions such as strcmp() can correctly compare strings.
 +    const int result = (ignoreCase ? VL_STRCASECMP : std::strcmp)(lhs.c_str(), rhs.c_str());

Unless the compiler is smart this will require a conditional indirect branch which is slow. Better to be safe:

      const int result = (ignoreCase ? VL_STRCASECMP(lhs.c_str(), rhs.c_str());
                          : std::strcmp(lhs.c_str(), rhs.c_str());

Ditto in V3Number.

 +extern IData VL_ATOI_N(std::string str, int base) VL_PURE; //pass by value because edit its content

It's preferable to take in a const reference, then first line of the function makes a string to edit from the argument. The runtime cost will be no worse.

 +++ b/src/V3AstNodes.h
 @@ -4811,6 +4811,44 @@ public:
 +class AstAtoN : public AstNodeUniop {
 +    // string.atoi(), atobin(), atohex(), atooct(), atoireal()
 +public:
 +    enum fmt_type {AtoI=10, AtoHex=16, AtoOct=8, AtoBin=2, AtoReal=-1};

Please capitalize enum values. StudlyCaps all types.

      enum FmtType {ATOI = 10, ATOHEX = 16, ATOOCT = 8, ATOBIN = 2, ATOREAL = -1};

 +    virtual string emitVerilog() {
 +        switch(m_fmt) {
 +            case AtoI: return "%l.atoi()";
 +            case AtoHex: return "%l.atohex()";
 +            case AtoOct: return "%l.atooct()";
 +            case AtoBin: return "%l.atobin()";
 +            case AtoReal: return "%l.atoreal()";
 +            default:V3ERROR_NA;

Please add a name() method that returns e.g. "atoi"/"atohex"/etc. That way it will show up in .tree files to help debug. You can then use name() in emitVerilog.

Also add name() "icompare"/"compare" to AstCompareNN.

 +++ b/src/V3Number.cpp
 @@ -1237,6 +1237,34 @@ V3Number& V3Number::opLogEq(const V3Number& lhs, const V3Number& rhs) {
 +V3Number& V3Number::opAtoN(const V3Number& lhs, int base) {
 +    UASSERT(base == -1 || base == 2 || base == 8 || base == 10 || base == 16,
 +            "base must be one of -1, 2, 8, 10, or 16.");

Please test against ATOF instead of -1.

 +    if(base < 0) return setDouble(std::atof(str.c_str()));

Please test against ATOF instead of -1.

 +++ b/src/V3Width.cpp
 @@ -2055,14 +2076,29 @@ private:
          } else if (nodep->name() == "atobin"
                     || nodep->name() == "atohex"
                     || nodep->name() == "atoi"
                     || nodep->name() == "atooct"
 -                   || nodep->name() == "atoreal"
 -                   || nodep->name() == "compare"
 -                   || nodep->name() == "icompare"
 -                   || nodep->name() == "getc"
 +                   || nodep->name() == "atoreal") {
 +            const char f = nodep->name()[3];
 +            const AstAtoN::fmt_type fmt = f == 'b' ? AstAtoN::AtoBin
 +                : f == 'h' ? AstAtoN::AtoHex
 +                : f == 'i' ? AstAtoN::AtoI
 +                : f == 'o' ? AstAtoN::AtoOct
 +                : AstAtoN::AtoReal;

This works but seems a little risky if new methods need adding. Suggest it's better to compare the full names again and choose appropriately, with an else.

These patches are looking good, so suggest you make these changes then continue to getc/putc on another branch, so these (minus getc/putc) can get merged to master.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 9, 2019


Original Redmine Comment
Author Name: Yutetsu TAKATSUKASA (@yTakatsukasa)
Original Date: 2019-12-09T14:39:57Z


Thanks again for the comments.

I have updated the branch other than the following item.

Adding these will slow down compiles, so please move it to verilated_heavy.cpp, then update the needHeavy check to require heavy if ATOI is present.
Let me confirm a bit about this item.

The change is in .cpp, not in .h. So the compilation time does not change a lot.
verilated_heavy.cpp does not exist at this moment. Do you mean I should create new .cpp file ?
There are functions which are declared in verilated_heavy.h and implemented in verilated.cpp.
Should I also move those functions (such as VL_TO_STRING()) to verilated_havy.cpp then ?

And I have a few questions as a newbie to this project.

  1. Which C++ standard is allowed ? C++03, C++11, or newer... though I assume C++03 after reading code.

  2. Is there suitable .cpp to share the implementation among compilation-time and simulation-time ?

    e.g. V3Number::opCompareNN() can be simpler and less redundant if the function can refer VL_CMP_NN() in verilated_heavy.h.
    But only include/verilatedos.h seems referred by src/*.cpp.

After I understand verilated_heavy.cpp topic, I will submit a PR which does not include getc/putc.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 9, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-09T17:40:20Z


I'm sorry, I misread the added includes as being in the .h file. Adding includes is fine in the verilated.cpp file.

We're at C++03 until at least mid/late next year. Threading though requires C++11.

There currently isn't any function sharing between verilation and runtime, excluding verilatedos.h, as the inputs to e.g. the math functions have had different requirements. If there's new functions that would benefit from being in both (and aren't low level compiler/OS appropriate for verilatedos.h) a new shared file can certainly be added.

Will look at your patch updates tonight and if good merge them in.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 10, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-10T00:20:51Z


This is merged, great work. FYI I made two minor changes, fixed some spacing to be closer to clang-format, and avoided using c_str on a temporary.

BTW I realize I forgot to have an ifdef'ed out test and V3Width check for substr (IEEE 6.16.8), hopefully you could implement that too?

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 10, 2019


Original Redmine Comment
Author Name: Yutetsu TAKATSUKASA (@yTakatsukasa)
Original Date: 2019-12-10T11:17:49Z


Thanks a lot!
Now I understand this project better.
I will work on getc/putc/substr towards this weekend.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 14, 2019


Original Redmine Comment
Author Name: Yutetsu TAKATSUKASA (@yTakatsukasa)
Original Date: 2019-12-14T23:53:28Z


I added the remaining methods (substr,putc, and getc()).
https://github.com/yTakatsukasa/verilator/commits/remaining_str_methods

Could you take a look at the branch ?

BTW do you prefer to be notified via PR on GitHub ?

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 15, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-15T01:35:49Z


You can do a pull request if you prefer, but post here mentioning it.

Great stuff, again!

 +++ b/include/verilated.cpp
 @@ -1826,6 +1826,30 @@ std::string VL_CVT_PACK_STR_NW(int lwords, WDataInP lwp) VL_MT_SAFE {
      return std::string(destout, len);
  }

 +std::string VL_PUTC_NSS(const std::string& lhs, int rhs, vluint8_t ths) VL_PURE {

Think can be:
+std::string VL_PUTC_N(const std::string& lhs, IData rhs, CData ths) VL_PURE {

Not sure what the "SS" suffix was for. Arguments that are generated by
V3Emit should generally always be a string or *Data. Similar other
functions and AstPutcNSS renamed to AstPutcN, etc.

 +++ b/src/V3AstNodes.h
 +class AstPutcNSS : public AstNodeTriop {
 ...
 +    virtual bool cleanOut() const { return false; }
 +    virtual bool cleanLhs() const { return false; }

cleanOut and cleanLhs must be true. All strings are always clean.
Same for Getc and Substr.

 +++ b/src/V3Const.cpp
 @@ -2526,6 +2526,8 @@ private:
 +    TREEOPC("AstPutcNSS{$lhsp.castConst, $rhsp.castConst, $thsp.castConst}",  "replaceConst(nodep)");
 +    TREEOPC("AstSubstrNSS{$lhsp.castConst, $rhsp.castConst, $thsp.castConst}",  "replaceConst(nodep)");

Getc also should be here, or does the default methods handle that?

 +++ b/src/V3EmitC.cpp
 @@ -643,6 +643,10 @@ public:
 +    virtual void visit(AstNodeTriop* nodep) {
 +        UASSERT(!emitSimpleOk(nodep), "Triop can not be described in a simple way");

s/can not/cannot/

Use UASSERT_OBJ instead, so errors get attributed to nodep.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 15, 2019


Original Redmine Comment
Author Name: Yutetsu TAKATSUKASA (@yTakatsukasa)
Original Date: 2019-12-15T03:28:42Z


Thanks for the comment.
I just updated in the following commit of the branch.
yTakatsukasa@987d74a

Getc also should be here, or does the default methods handle that?

I understand that the following line takes care of AstPutcN.

2247     TREEOPC("AstNodeBiop {$lhsp.castConst, $rhsp.castConst}",  "replaceConst(nodep)");

Verilated C++ code also tells that Putc is resolved if the inputs of the method are constant.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Dec 15, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-15T13:14:59Z


Excellent, pushed to git towards eventual 4.026 release.

If you're willing to work on something else next it would be great. You could do one of the bunch of support bugs similar to this one, or work on any other bug, or any missing feature of your interest. It all would all be appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.