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

[DSLX] In struct parametrics parametric argument types other than u32 should be allowed, including s32 #1841

Open
dank-openai opened this issue Jan 10, 2025 · 1 comment
Labels
dslx DSLX (domain specific language) implementation / front-end

Comments

@dank-openai
Copy link

dank-openai commented Jan 10, 2025

Describe the bug

This does not work:

pub struct FixedPoint<NUM_BITS: u32, BINARY_EXPONENT: s32> { 
    data: uN[NUM_BITS], 
}
#[test]
fn test_neg_exp() {
    let a = FixedPoint<u32:2, s32:-1> { data: u8:0 };
}

because BINARY_EXPONENT is s32. It works if I change it to u32. I really do want a signed value, so this is not acceptable.

Error message, referring to BINARY_EXPONENT when I create a:

sN[32] vs uN[32]: Dimension s32:-1 must be a `u32` (soon to be `usize`, see https://github.com/google/xls/issues/450 for details).

Note that other parametric argument types which are useful to implementers are also disallowed, e.g. bool would have been useful to me to make a signed magnitude workaround:

pub struct FixedPoint<NUM_BITS: u32, EXPONENT_IS_NEGATIVE:bool, BINARY_UEXPONENT: u32>

but the above is also not allowed.

To Reproduce

Use the code above, try to compile.

Expected behavior

That I can make BINARY_EXPONENT signed without issues.

This would be very handy for making an open source fixed point library.

@dank-openai dank-openai changed the title impl erroneously requires second template argument to be u32 s32 parametric arguments should be allowed Jan 13, 2025
@dank-openai dank-openai changed the title s32 parametric arguments should be allowed parametric argument types other than u32 should be allowed, including s32 Jan 13, 2025
@cdleary cdleary added the dslx DSLX (domain specific language) implementation / front-end label Jan 14, 2025
@erinzmoore
Copy link
Collaborator

erinzmoore commented Jan 15, 2025

Taking a look at this, but given the state of type system v2, if this proves to be very involved, the fix may wait for v2. Oh, re-reading this I see it isn't specific to impl, but to parametric types in all cases. So, this will in fact be blocked on type system v2. Unassigning

@erinzmoore erinzmoore assigned erinzmoore and unassigned erinzmoore Jan 15, 2025
@cdleary cdleary changed the title parametric argument types other than u32 should be allowed, including s32 [DSLX] In struct parametrics parametric argument types other than u32 should be allowed, including s32 Jan 17, 2025
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
Projects
Status: No status
Development

No branches or pull requests

3 participants