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

zero! should support parametric structs and arrays of structs #984

Closed
proppy opened this issue May 26, 2023 · 6 comments
Closed

zero! should support parametric structs and arrays of structs #984

proppy opened this issue May 26, 2023 · 6 comments
Labels
dslx DSLX (domain specific language) implementation / front-end enhancement New feature or request
Milestone

Comments

@proppy
Copy link
Member

proppy commented May 26, 2023

https://google.github.io/xls/dslx_reference/#zerot doesn't seems to work with parametric strucst and arrays of structs.

Is that a known limitation, are there possible workaround?

@proppy proppy changed the title zero! builtin don't work with parametric structs and arrays of structs. add zero! support for parametric structs and arrays of structs May 26, 2023
@proppy proppy changed the title add zero! support for parametric structs and arrays of structs zero! should support parametric structs and arrays of structs May 26, 2023
@cdleary
Copy link
Collaborator

cdleary commented May 26, 2023

Re: #982 -- we generally don't want to introduce more backtracking into the parser as an implementation strategy, since it makes for bad error messages and slow parsing. I was hoping to restructure the grammar around types a bit, this is one of the first instances where we have parametric types so the grammar was a bit overfit for parametric-values-only.

The test cases looked good though, maybe those can be used with a DISABLED_ and a TODO referencing this issue?

See also related #904

@cdleary cdleary added enhancement New feature or request dslx DSLX (domain specific language) implementation / front-end labels May 26, 2023
@cdleary
Copy link
Collaborator

cdleary commented May 26, 2023

Oh, and the workaround is to define a type alias and then use that as the parametric argument to zero!

mtdudek added a commit to antmicro/xls that referenced this issue May 26, 2023
This commit adds tests to improve
`zero!<>()` coverage. Some tests are disabled
(GH-google#984)

Signed-off-by: Maciej Dudek <[email protected]>
@mtdudek
Copy link
Contributor

mtdudek commented May 26, 2023

I've created PR #985 with tests and disabled ones that don't work right now.
I think #976 can benefit from adding type parsing to parametric pass.

I've also created draft PR #986 with a possible solution to parse parametric types.
It doesn't parse :: after first < or [, it also doesn't allow for using
fields from const arrays, like:

const a = u32[2] : {1,2}

fn b<A:u32>(x: u32) -> u32 {
  x + A
}

b<a[0]>(a[1])

As far as I can tell, using const array's fields was never allowed, so proposed solution doesn't break it.

@mtdudek mtdudek moved this to In Progress in Antmicro XLS May 26, 2023
@proppy
Copy link
Member Author

proppy commented May 30, 2023

@mtdudek is that blocking #994 and #995?

@mtdudek
Copy link
Contributor

mtdudek commented May 30, 2023

It's not blocking. There is workaround to pass any type to zero!<> using type definitions.

mtdudek added a commit to antmicro/xls that referenced this issue May 31, 2023
This commit adds tests to improve
`zero!<>()` coverage. Some tests are disabled
(GH-google#984)

Signed-off-by: Maciej Dudek <[email protected]>
@mikex-oss
Copy link
Collaborator

mikex-oss commented Jun 18, 2024

+1. Just ran into this myself.

In my case, the zero! is within a parametric function intended specifically to adjust the array size of the struct. So it's not really viable to make an explicit type without making the function non-parameteric.

@meheff meheff added this to the DSLX Next milestone Jul 17, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Antmicro XLS Jul 18, 2024
@proppy proppy moved this to Done in XLS usability Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dslx DSLX (domain specific language) implementation / front-end enhancement New feature or request
Projects
Status: Done
Development

No branches or pull requests

5 participants