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

Jump to bottom

AstReplicate constructor consistency #738

Closed
veripoolbot opened this issue Apr 10, 2014 · 3 comments
Closed

AstReplicate constructor consistency #738

veripoolbot opened this issue Apr 10, 2014 · 3 comments
Labels
area: configure/compiling Issue involves configuring or compilating Verilator itself resolution: fixed Closed; fixed

Comments

@veripoolbot
Copy link
Contributor

Author Name: Glen Gibb
Original Redmine Issue: 738 from https://www.veripool.org
Original Date: 2014-04-10
Original Assignee: Glen Gibb

I have noticed inconsistency between the two AstReplicate constructors:

struct AstReplicate : public AstNodeBiop {
     // Verilog {rhs{lhs}} - Note rhsp() is the replicate value, not the lhsp()
     AstReplicate(FileLine* fl, AstNode* lhsp, AstNode* rhsp) : AstNodeBiop(fl, lhsp, rhsp) {
         if (AstConst* constp=rhsp->castConst()) {
             dtypeSetLogicSized(lhsp->width()*constp->toUInt(), lhsp->width()*constp->toUInt(), AstNumeric::UNSIGNED);
         }
     }
     AstReplicate(FileLine* fl, AstNode* lhsp, uint32_t repCount)
	: AstNodeBiop(fl, lhsp, new AstConst(fl, repCount)) {}

The first constructor sets the data type width (lhsp's width * rhsp's replication count), while the second constructor does not set the width. While this is probably taken care of by the V3Width visitor, I'd suggest adding the following call to dtypeSetLogicSized in the second constructor:

dtypeSetLogicSized(lhsp->width()*repCount, lhsp->width()*repCount, AstNumeric::UNSIGNED);

Alternatively, this common code could be shifted to an init() method, or delegating constructors could be used if you want to use C++11 language features.
@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Glen Gibb
Original Date: 2014-04-10T17:51:13Z

I've pushed two alternative fixes to my github repo: https://github.com/grg/verilator.git

The alternatives are:

  • branch @#�_a@: move common code to an init() function
  • branch @#�_b@: add call to dtypeSetLogicSized() in second constructor

@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2014-04-10T21:57:11Z

init() seems nicer. I added a check that lhs is defined to avoid a coredump.

Pushed to git towards 3.857.

@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2014-04-10T21:58:39Z

P.S. While I would like to use C++11 it'll probably unfortunately be 5-10 years before it's common enough to use it in portable code.

@veripoolbot veripoolbot added area: configure/compiling Issue involves configuring or compilating Verilator itself resolution: fixed Closed; fixed labels Dec 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area: configure/compiling Issue involves configuring or compilating Verilator itself resolution: fixed Closed; fixed
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@veripoolbot