Added cast to type support

[rherveille]

Added support for casting to type; e.g. assign a = int'(some_var); assign b = my_type_def'(some_var);

[rherveille]

I do have a few tests. How do you want me to include them in the pull request?

Richard

From: Catherine @.> Date: Friday, 15 March 2024 at 11:59 To: YosysHQ/yosys @.> Cc: Richard Herveille @.>, Author @.> Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)

@whitequark commented on this pull request.

No tests?

— Reply to this email directly, view it on GitHubhttps://github.com/YosysHQ/yosys/pull/4284#pullrequestreview-1940117522, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYKSCILLFQNPE7SK2HI4WLYYNAIRAVCNFSM6AAAAABEYQAKHGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNBQGEYTONJSGI. You are receiving this because you authored the thread.Message ID: @.***>

[rherveille]

Tests added to the tests/verilog directory

Richard

From: Richard Herveille @.> Date: Friday, 15 March 2024 at 14:13 To: YosysHQ/yosys @.> Cc: Richard Herveille @.***> Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284) I do have a few tests. How do you want me to include them in the pull request?

Richard

From: Catherine @.> Date: Friday, 15 March 2024 at 11:59 To: YosysHQ/yosys @.> Cc: Richard Herveille @.>, Author @.> Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)

@whitequark commented on this pull request.

No tests?

— Reply to this email directly, view it on GitHubhttps://github.com/YosysHQ/yosys/pull/4284#pullrequestreview-1940117522, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYKSCILLFQNPE7SK2HI4WLYYNAIRAVCNFSM6AAAAABEYQAKHGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNBQGEYTONJSGI. You are receiving this because you authored the thread.Message ID: @.***>

[rherveille]

Done

On Mar 17, 2024, at 09:49, Catherine @.***> wrote:



@whitequark commented on this pull request.

Could you also add a test for a cast to a user defined struct please?

— Reply to this email directly, view it on GitHubhttps://github.com/YosysHQ/yosys/pull/4284#pullrequestreview-1941530653, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYKSCLIJIL4A42HIUA4FQTYYXCSBAVCNFSM6AAAAABEYQAKHGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNBRGUZTANRVGM. You are receiving this because you authored the thread.Message ID: @.***>

[rherveille]

Hi Zach,

See my answers inline.

• I think this patch is already >70% of the work needed to also support:

Ø x = (struct_t'(y)).z; Ø I'm sure we'd gladly accept a patch adding support if you're interested!

Yes, but I am working on enhancing ‘import’ first. I also noticed that my_struct_variable.my_struct_variable.my_struct_variable is not supported. That’s on the todo list as well. Cheers, Richard

From: Zachary Snow @.> Date: Wednesday, 22 May 2024 at 17:55 To: YosysHQ/yosys @.> Cc: Richard Herveille @.>, Author @.> Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)

@zachjs requested changes on this pull request.

I think substantively this change is in a great state. I think nearly all of my comments are minor or stylistic. Sorry for the delay in getting this reviewed!

I am excited for Yosys to gain this feature! Thank you for your contribution!

I think this patch is already >70% of the work needed to also support:

x = (struct_t'(y)).z;

I'm sure we'd gladly accept a patch adding support if you're interested!

---

In frontends/ast/ast.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610806809:

• switch (children[0]->type)

• {

• case AST_WIRE:

• if (children[0]->children.size() > 0) {

• children[0]->children[0]->dumpVlog(f, "");

• fprintf(f, "'(");

• children[1]->dumpVlog(f, "");

• fprintf(f, ")");

• } else {

• fprintf(f, "%d'(", children[0]->range_left - children[0]->range_right +1);

• children[1]->dumpVlog(f, "");

• fprintf(f, ")");

• }

• break;

• default: //AST_IDENTIFIER

I assume this branch covers at least AST_CONSTANT, too. What am I missing?

[rih] The default statement covers that. It was already supported, I didn’t change any of that code. I merely added AST_WIRE

---

In frontends/ast/ast.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610809183:

• fprintf(f, "'(");

• children[1]->dumpVlog(f, "");

• fprintf(f, ")");

Arguably these three lines are now repeated three times. Can we do this once at the end instead?

[rih] Done

---

In frontends/ast/ast.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610809746:

         break;

+

There is a stray newline here.

---

In frontends/ast/simplify.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610809776:

@@ -1487,6 +1487,7 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin

                   width_hint = max(width_hint, children[1]->range_left - children[1]->range_right + 1);

           }

           break;

+

There is a stray newline here.

[rih] No, a newline was missing in the original file

---

In tests/verilog/size_cast.svhttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610810631:

@@ -15,6 +15,19 @@ module top;

 logic signed [1:0] L2sb10 = 2;

 logic signed [1:0] L2sb11 = 3;

• typedef logic u1bit_t;

• typedef logic signed s1bit_t;

• typedef logic [1:0] u2bit_t;

• typedef logic signed [1:0] s2bit_t;

• typedef logic [2:0] u3bit_t;

• typedef struct packed {

• u1bit_t sign;

• u3bit_t msbs;

• byte lsbs;

• } s12bit_packed_struct_t;

When I created this test, I don't think I intended to have any double-newlines breaking up portions of the test. Would you mind sticking to the existing style here?

[rih] Does it matter? People use double-newlines to create blocks in Verilog code all the time. It helped me to understand which tests were related. But I removed the double newlines, because you asked.

---

In frontends/ast/simplify.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610813480:

+

• //delete child's children

I think it's best to avoid comments that simply turn a line of code into a sentence. For example, I don't think //delete child's children clarifies child->delete_children(); at all. ⬇️ Suggested change

-

• //delete child's children

---

In frontends/ast/simplify.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610816272:

+

• //create AST_CONSTANT node ⬇️ Suggested change

• //create AST_CONSTANT node

---

In frontends/ast/simplify.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610816402:

+

• // Ensure typedef itself is fully simplified

• while (template_node->simplify(const_fold, stage, width_hint, sign_hint)) {};

• switch (template_node->type)

• {

• case AST_WIRE: {

• if (template_node->children.size() > 0 && template_node->children[0]->type == AST_RANGE)

• width = range_width(this, template_node->children[0]);

• //delete child's children

• child->delete_children();

• //create AST_CONSTANT node

• node = mkconst_int(width, true);

• ⬇️ Suggested change

---

In frontends/ast/simplify.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610816615:

• switch (template_node->type)

• {

• case AST_WIRE: {

• if (template_node->children.size() > 0 && template_node->children[0]->type == AST_RANGE)

• width = range_width(this, template_node->children[0]);

• //delete child's children

• child->delete_children();

• //create AST_CONSTANT node

• node = mkconst_int(width, true);

• break;

• }

• case AST_STRUCT: {

Similar changes in this block.

---

In frontends/ast/simplify.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610816835:

+

• //insert AST_CONSTANT ⬇️ Suggested change

• //insert AST_CONSTANT

---

In frontends/ast/simplify.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610816911:

• //delete child's children

• child->delete_children();

• //create AST_CONSTANT node

• width = size_packed_struct(template_node, 0);

• node = mkconst_int(width, false);

• break;

• }

• default:

• log_error("Don't know how to translate static cast of type %s\n", type2str(template_node->type).c_str());

• }

• }

• //Remove child node ⬇️ Suggested change

• //Remove child node

---

In frontends/ast/simplify.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610819253:

• default:

• log_error("Don't know how to translate static cast of type %s\n", type2str(template_node->type).c_str());

• }

• }

• //Remove child node

• delete child;

• children.erase(children.begin());

• //insert AST_CONSTANT

• children.insert(children.begin(),node);

• }

• detect_width_simple = true;

• children_are_self_determined = true;

• ⬇️ Suggested change

I recognize comments like these are pretty nit-picky! That said, I think it's important to adhere roughly to the existing style given that Yosys has a relatively broad base of contributors.

[rih] I find a lot of the code seriously lacking in documentation. Especially these complex structures. But that’s a personal preference.

---

In frontends/ast/simplify.cchttps://github.com/YosysHQ/yosys/pull/4284#discussion_r1610819580:

+

• //Construct AST_CONSTANT node ⬇️ Suggested change

• //Construct AST_CONSTANT node

— Reply to this email directly, view it on GitHubhttps://github.com/YosysHQ/yosys/pull/4284#pullrequestreview-2072606221, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYKSCM6ZHDXXAT6L2NRD2LZDU46BAVCNFSM6AAAAABEYQAKHGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANZSGYYDMMRSGE. You are receiving this because you authored the thread.Message ID: @.***>

[rherveille]

Any update on the pull request? All requested changes have been implemented.

[zachjs]

To short-circuit another round of reviews and edits, I've made some edits locally. I'd like to add them to this PR and have you take a look before I merge them. Can you either enable "Maintainers are allowed to edit this pull request." or pull in my changes at https://github.com/zachjs/yosys/tree/cast_type?

[rherveille]

I am not using a personal account. Github doesn’t allows "Maintainers are allowed to edit this pull request." To non-personal accounts. I will need to find time to pull in your changes. But time is currently extremely limited.

Richard

From: Zachary Snow @.> Date: Sunday, 1 September 2024 at 06:50 To: YosysHQ/yosys @.> Cc: Richard Herveille @.>, Author @.> Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)

To short-circuit another round of reviews and edits, I've made some edits locally. I'd like to add them to this PR and have you take a look before I merge them. Can you either enable "Maintainers are allowed to edit this pull request." or pull in my changes at https://github.com/zachjs/yosys/tree/cast_type?

— Reply to this email directly, view it on GitHubhttps://github.com/YosysHQ/yosys/pull/4284#issuecomment-2323348635, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYKSCNNONLUYF3KDNXZOSLZUMLR5AVCNFSM6AAAAABEYQAKHGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRTGM2DQNRTGU. You are receiving this because you authored the thread.Message ID: @.***>

[rherveille]

Hi Zach,

I manually reviewed the local changes you made to the cast_type pull request and I see no issues with your changes. Do you still need me to update my pull request?

Richard

From: Richard Herveille @.> Date: Tuesday, 3 September 2024 at 09:17 To: YosysHQ/yosys @.>, YosysHQ/yosys @.> Cc: Author @.>, Richard Herveille @.***> Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)

I am not using a personal account. Github doesn’t allows "Maintainers are allowed to edit this pull request." To non-personal accounts. I will need to find time to pull in your changes. But time is currently extremely limited.

Richard

From: Zachary Snow @.> Date: Sunday, 1 September 2024 at 06:50 To: YosysHQ/yosys @.> Cc: Richard Herveille @.>, Author @.> Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)

To short-circuit another round of reviews and edits, I've made some edits locally. I'd like to add them to this PR and have you take a look before I merge them. Can you either enable "Maintainers are allowed to edit this pull request." or pull in my changes at https://github.com/zachjs/yosys/tree/cast_type?

— Reply to this email directly, view it on GitHubhttps://github.com/YosysHQ/yosys/pull/4284#issuecomment-2323348635, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYKSCNNONLUYF3KDNXZOSLZUMLR5AVCNFSM6AAAAABEYQAKHGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRTGM2DQNRTGU. You are receiving this because you authored the thread.Message ID: @.***>

[zachjs]

Yes, could you please apply or cherry-pick my patch?

[rherveille]

Only the edits to my files, right?

Richard

From: Zachary Snow @.> Date: Monday, 23 September 2024 at 17:25 To: YosysHQ/yosys @.> Cc: Richard Herveille @.>, Author @.> Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)

Yes, could you please apply or cherry-pick my patch?

— Reply to this email directly, view it on GitHubhttps://github.com/YosysHQ/yosys/pull/4284#issuecomment-2369845990, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYKSCN3OKFDVL3DCCPSWEDZYCWQ3AVCNFSM6AAAAABEYQAKHGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRZHA2DKOJZGA. You are receiving this because you authored the thread.Message ID: @.***>

[zachjs]

Yes. To be clear, I'm imagining you'll run something like:

git fetch https://github.com/zachjs/yosys.git cast_type
git cherry-pick FETCH_HEAD
(build, test, etc.)
git push

[rherveille]

Done, the code builds and all tests pass

Richard

From: Zachary Snow @.> Date: Monday, 23 September 2024 at 20:23 To: YosysHQ/yosys @.> Cc: Richard Herveille @.>, Author @.> Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)

Yes. To be clear, I'm imagining you'll run something like:

git fetch https://github.com/zachjs/yosys.git cast_type

git cherry-pick FETCH_HEAD

(build, test, etc.)

git push

— Reply to this email directly, view it on GitHubhttps://github.com/YosysHQ/yosys/pull/4284#issuecomment-2370033311, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYKSCMCMLMC4AM2FVO3NJLZYDLJDAVCNFSM6AAAAABEYQAKHGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNZQGAZTGMZRGE. You are receiving this because you authored the thread.Message ID: @.***>

[rherveille]

[like] Richard Herveille reacted to your message:

---

From: Zachary Snow @.> Sent: Sunday, September 29, 2024 9:03:22 PM To: YosysHQ/yosys @.> Cc: Richard Herveille @.>; Author @.> Subject: Re: [YosysHQ/yosys] Added cast to type support (PR #4284)

Merged #4284https://github.com/YosysHQ/yosys/pull/4284 into main.

— Reply to this email directly, view it on GitHubhttps://github.com/YosysHQ/yosys/pull/4284#event-14446656291, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADYKSCP5JZQ4SSWV6HEHNFTZZBTJVAVCNFSM6AAAAABEYQAKHGVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJUGQ2DMNRVGYZDSMI. You are receiving this because you authored the thread.Message ID: @.***>