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 indexed part select support #123

Merged
merged 1 commit into from
Aug 17, 2018
Merged

add indexed part select support #123

merged 1 commit into from
Aug 17, 2018

Conversation

rroohhh
Copy link
Contributor

@rroohhh rroohhh commented Aug 12, 2018

This adds support for constant width variable offset slices called indexed part notation in verilog.

@sbourdeauducq
Copy link
Member

The more special Verilog features we have, the more complex the code is to test and maintain.
Also, certain cores written would use this indexed part select, whereas others would not. I prefer keeping a single way to do things as much as possible.
This also makes it more complicated to support other backends in the future such as VHDL.
As you indicated on IRC, the benefit of doing this is more readable output. I disagree with this, as having explicit slice boundaries in the generated code makes it easier to spot certain bugs.

Having a part method that returns a _Slice would be OK, on the other hand.

@rroohhh
Copy link
Contributor Author

rroohhh commented Aug 13, 2018

The problem with slice is you can't use non constant slice boundaries, here the offset can be dynamic. This can be converted to a shift and a slice, but due to the lowering of complex slices the shift always gets extracted to a assign which sometimes makes it difficult to read imo.

Im not sure how you would convert this into just a slice.

@sbourdeauducq
Copy link
Member

That's a good point. Can you add a docstring to part and mention this?
What about signedness and sign extension, did you check that it does the right thing?
Then it seems fine to me.

@rroohhh
Copy link
Contributor Author

rroohhh commented Aug 14, 2018

I added a docstring, however I'm not sure how signedness and sign extension would apply here.

@sbourdeauducq
Copy link
Member

I'm not sure how signedness and sign extension would apply here

For example, what happens when you add a Part with a signed signal that is smaller than the Part?

@rroohhh
Copy link
Contributor Author

rroohhh commented Aug 15, 2018

This does the exact same as a normal part select / _Slice would, it is treated as unsigned by verilog. This is also what is returned by value_bits_sign for a _Part so everything should be correct.

@sbourdeauducq
Copy link
Member

Thanks.

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

Successfully merging this pull request may close these issues.

None yet

2 participants